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

Make the initializer_list overload of Subarray::set_subarray zero-copy. #4062

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented Apr 25, 2023

Instead of copying it first to a temporary vector, we use std::data to directly pass the initializer_list's data to the C API.

This is the only use of initializer_list in the C++ API, excluding the deprecated Query::set_subarray which was not modified.


TYPE: CPP_API
DESC: Make the initializer_list overload of Subarray::set_subarray zero-copy.

@eric-hughes-tiledb
Copy link
Contributor

Can I do the same with the overload that takes an std::vector<std::array<T,2>>? Is it guaranteed that the elements are laid out right next to each other in memory?

Don't need a guarantee when unit tests can verify it.

As a rule it's a mistake in C++ to assume any particular memory layout. Alignment, padding, and compiler behavior all come into play.

@teo-tsirpanis
Copy link
Member Author

Or even better a static_assert. This would block at compile-time types with weird layout from being used (like a struct {uint8_t x; uint16_t y;} which will very likely need a padding byte).

@teo-tsirpanis teo-tsirpanis requested a review from ihnorton June 8, 2023 07:11
@teo-tsirpanis
Copy link
Member Author

Can we merge it as it is? The other overload can be done in a future PR.

@KiterLuc KiterLuc merged commit b5c11a4 into TileDB-Inc:dev Oct 9, 2023
50 checks passed
@teo-tsirpanis teo-tsirpanis deleted the cppapi-zero-copy branch October 9, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants