Skip to content

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

Merged
rroelke merged 5 commits intomainfrom
rr/attribute-enumeration-name-ref
Jun 30, 2025
Merged

chore: Attribute::get_enumeration_name returns a reference rather than a copy in order to add Rust binding#5567
rroelke merged 5 commits intomainfrom
rr/attribute-enumeration-name-ref

Conversation

@rroelke
Copy link
Copy Markdown
Contributor

@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

Comment thread cmake/oxidize.cmake
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
Copy Markdown
Contributor 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
Copy Markdown
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