Skip to content

chore: Attribute::get_enumeration_name returns a reference rather than a copy in order to add Rust binding #5567

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

Merged
merged 5 commits into from
Jun 30, 2025

Conversation

rroelke
Copy link
Member

@rroelke rroelke commented Jun 29, 2025

This is split off of #5566 in order to keep that pull request more focused.

Differences in copy and move semantics between Rust and C++ mean that they are generally not very good at passing values across the boundary - most of the time it is necessary to pass references instead.

#5566 intends to call Attribute::get_enumeration_name from Rust. Prior to these changes, that function returns std::optional<std::string>, which cannot be passed across the boundary (neither std::optional nor std::string can be). We can either pass a pointer to a string, or pass a string contained within a smart pointer.

To avoid the additional memory allocations required for both copying the std::string and placing it in a unique_ptr, we choose the former.

However, std::optional does not naturally support references, so we cannot do std::optional<std::string&>. Instead we change the return type to std::optional<std::reference_wrapper<std::string>>.

In addition to enabling passing the result of this function to Rust without allocating additional memory, this also avoids making additional copies of the string. This is not likely to matter from a performance perspective, but is still nice!


TYPE: NO_HISTORY
DESC: Rust binding for Attribute::get_enumeration_name

@@ -306,10 +306,9 @@ if (TILEDB_RUST)
set(bridge_h_src "${bridge_generated_dir}/src/${bridge}.h")
set(bridge_cc_src "${bridge_generated_dir}/src/${bridge}.cc")

string(REPLACE "-" "_" bridge_sanitize_relpath ${bridge_sanitize_relpath})
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not directly related to the PR description but was causing problems in some of my build environments.

I was building in $HOME/tiledb/TileDB-4 and the previous code was replacing that with $HOME/tiledb/TileDB_4 which was messing up include paths.

This correction only makes this substitution for the path components after tiledb/oxidize.

Copy link
Member

@kounelisagis kounelisagis left a comment

Choose a reason for hiding this comment

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

LGTM!

@rroelke rroelke merged commit 7c9db9a into main Jun 30, 2025
56 checks passed
@rroelke rroelke deleted the rr/attribute-enumeration-name-ref branch June 30, 2025 13:58
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.

2 participants