Skip to content

Conversation

@eramongodb
Copy link
Contributor

Resolves CXX-3237 and CXX-3238 for the v1::data_key_options component.

Due to T const& return types in the v_noabi API, v_noabi::options::data_key cannot be refactored in terms of v1::data_key_options. Therefore, only v_noabi <-> v1 conversion functions are defined. Aside from the inlining of most member functions (to reduce the v_noabi ABI footprint), the implementation for v_noabi largely remains unchanged.

Note: view_or_value<T> only exposes a view of its managed object; v_noabi -> v1 is therefore copy-only.

@eramongodb eramongodb self-assigned this Nov 17, 2025
@eramongodb eramongodb requested a review from a team as a code owner November 17, 2025 20:37
Copy link
Collaborator

@connorsmacd connorsmacd left a comment

Choose a reason for hiding this comment

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

LGTM! Just one comment with minor suggestions.

Comment on lines 130 to 132
for (auto const& name : _impl._key_alt_names) {
names.push_back(const_cast<char*>(name.c_str())); // For copy only.
}
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
for (auto const& name : _impl._key_alt_names) {
names.push_back(const_cast<char*>(name.c_str())); // For copy only.
}
std::transform(
_impl._key_alt_names.begin(), _impl._key_alt_names.end(), std::back_inserter(names), [](auto const& name) {
return const_cast<char*>(name.c_str()); // For copy only
});

I tend to advocate for algorithm use, but with a single-statement loop like this, I chalk it up to preference.

Also, I am not sure what your intentions are with clang-tidy, but you may want to preemptively add a // NOLINT(cppcoreguidelines-pro-type-const-cast).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to advocate for algorithm use, but with a single-statement loop like this, I chalk it up to preference.

Accepted the suggestion, with the caveat that generic lambdas are C++14 and newer. I've personally preferred to avoid <algorithm> unless dealing with generic code over iterators (e.g. no direct access to the parent container or knowledge of its type, if any), but there is an argument to be made for opting-into the opportunity for stdlib-internal optimizations based on iterator and element type (likely not applicable here, but nevertheless) and/or ease of extending behavior (e.g. C++17 parallel execution: this could be par_unseq). But maybe this too is YAGNI.

I am not sure what your intentions are with clang-tidy

I'm hesitating to insert any clang-tidy-related comments until we've decided on the set of diagnostics to care about in #1496. Feel free to add yourself as a reviewer if interested in providing input.

@eramongodb eramongodb merged commit 6433dac into mongodb:master Nov 19, 2025
5 of 6 checks passed
@eramongodb eramongodb deleted the cxx-abi-v1-data_key_options branch November 19, 2025 19:48
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