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

Implement typed_array #65

Merged
merged 3 commits into from
Apr 19, 2024
Merged

Conversation

Alex-PLACET
Copy link
Collaborator

@Alex-PLACET Alex-PLACET commented Apr 8, 2024

Fix #61

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.

Thank you, @Alex-PLACET.

Here is a first review.

include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

Thanks for this initial implementation. Here are a few comments, I find it easier to gather them rather than spreading them in the diff:

  • Now that Layouts now store a reference to array_data instead of a value #51 is merged, the layout classes' copy and move semantics are deleted. Therefore the typed_array class should implement proper semantics.
  • Nitpicking: Since the API is going to grow, it could be nice to organize the different methods in the same order as std::vector, with the same sections (element access, iterators, etc). Notice that I am following the same "convention" in the incoming refactoring of the buffer class.
  • Even if the buffer and the layout classes do not provide all the required methods to implement resize/insert/erase/clear, there are a couple of methods that can already be instantiated: front / back / data / empty (and I'm probably missing some of them)
  • I think comparison operators can also be implemented

@Alex-PLACET Alex-PLACET force-pushed the implement_typed_array branch 3 times, most recently from ec33d66 to faa1997 Compare April 12, 2024 08:24
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.

A review with some outdated comments, I believe.

test/test_typed_array.cpp Outdated Show resolved Hide resolved
include/sparrow/algorithm.hpp Outdated Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
include/sparrow/ordering.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Show resolved Hide resolved
include/sparrow/array_data.hpp Show resolved Hide resolved
include/sparrow/typed_array.hpp Show resolved Hide resolved
include/sparrow/typed_array.hpp Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
include/sparrow/ordering.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Show resolved Hide resolved
include/sparrow/details/3rdparty/float16_t.hpp Outdated Show resolved Hide resolved
include/sparrow/algorithm.hpp Outdated Show resolved Hide resolved
include/sparrow/compiler.hpp Outdated Show resolved Hide resolved
include/sparrow/compiler.hpp Outdated Show resolved Hide resolved
include/sparrow/ordering.hpp Outdated Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
test/array_data_creation.hpp Outdated Show resolved Hide resolved
test/array_data_creation.hpp Show resolved Hide resolved
test/test_algorithm.cpp Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
@Alex-PLACET Alex-PLACET force-pushed the implement_typed_array branch 2 times, most recently from 677f42f to 1567e1f Compare April 17, 2024 15:01
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.

Definitely LGTM.

I just have one comment.

include/sparrow/config.hpp Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
include/sparrow/algorithm.hpp Outdated Show resolved Hide resolved
include/sparrow/config.hpp Outdated Show resolved Hide resolved
include/sparrow/data_traits.hpp Show resolved Hide resolved
include/sparrow/typed_array.hpp Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
include/sparrow/typed_array.hpp Outdated Show resolved Hide resolved
test/array_data_creation.hpp Outdated Show resolved Hide resolved
test/test_typed_array.cpp Outdated Show resolved Hide resolved
test/test_variable_size_binary_layout.cpp Show resolved Hide resolved
@Alex-PLACET Alex-PLACET force-pushed the implement_typed_array branch 2 times, most recently from 47a9051 to c0d8ee3 Compare April 18, 2024 14:01
@Alex-PLACET Alex-PLACET merged commit 3f996ab into man-group:main Apr 19, 2024
21 checks passed
@Alex-PLACET Alex-PLACET deleted the implement_typed_array branch April 19, 2024 12:04
@SylvainCorlay
Copy link
Collaborator

Congrats on getting the PR in @Alex-PLACET !

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 typed_array
5 participants