Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented variable_size_binary_layout #32

Merged
merged 14 commits into from
Mar 26, 2024

Conversation

JohanMabille
Copy link
Collaborator

@JohanMabille JohanMabille commented Mar 19, 2024

Fix #39

test/test_array_data.cpp Outdated Show resolved Hide resolved
@JohanMabille JohanMabille force-pushed the variable_size_layout branch 2 times, most recently from 569d3db to 11d7b3b Compare March 21, 2024 09:09
@JohanMabille JohanMabille marked this pull request as ready for review March 21, 2024 09:10
include/sparrow/variable_size_binary_layout.hpp Outdated Show resolved Hide resolved
* - offset: [0, 6, 11, 13, 15, 24, 30]
* - data: ['p','l','e','a','s','e','a','l','l','o','w','m','e','t','o','i','n','t','r','o','d','u','c','e','m','y','s','e','l','f']
*
* @tparam T the type of the data
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in the example it is char? or is it const char * or string?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value type is string. They are all stored as a contiguous buffer of char. I've added some precisions in the documentation.

Copy link
Collaborator

@Klaim Klaim Mar 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like a clarification in that specific comment line, that here the value type is the one stored in the data buffer, and not the byte representation of it.

include/sparrow/variable_size_binary_layout.hpp Outdated Show resolved Hide resolved

TEST_SUITE("variable_size_binary_layout")
{
TEST_CASE_FIXTURE(vs_binary_fixture, "types")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a runtime test case when it's a bunch of compile-time checks.

I wonder if there is compile-time checks in doctest, I know there are in catch2 (I prefer doctest usually)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found such a feature in doctest. I can move these assertions to the fixture class itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that if any static assert needs to use that class, then the class must be defined before that use, so probably it's best to put them next but after that class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static asserts are on the layout_type defined in the fixture class, so I guess it's fine to keep the static assert in the fixture class?

test/test_variable_size_binary_layout.cpp Show resolved Hide resolved
include/sparrow/mp_utils.hpp Outdated Show resolved Hide resolved
include/sparrow/variable_size_binary_layout.hpp Outdated Show resolved Hide resolved
include/sparrow/variable_size_binary_layout.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another comment (I'll stop there).

include/sparrow/array_data.hpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mon ultime bafouille.

const_bitmap_iterator bitmap_cbegin() const;
const_bitmap_iterator bitmap_cend() const;

bool has_value(size_type i) const;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool has_value(size_type i) const;
bitmap_const_reference has_value(size_type i) const;

@JohanMabille JohanMabille mentioned this pull request Mar 26, 2024
@JohanMabille JohanMabille merged commit 9297e84 into man-group:main Mar 26, 2024
20 checks passed
@JohanMabille JohanMabille deleted the variable_size_layout branch March 26, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement variable_size_binary_layout
4 participants