-
Notifications
You must be signed in to change notification settings - Fork 548
v1: hint (CXX-3237, CXX-3238) #1516
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
Conversation
kevinAlbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
even though the behavior continues to be supported
Good catch. I agree with preserving the behavior (despite documentation) to avoid potentially breaking existing code.
| /// | ||
| /// Returns a types::bson_value::view representing this hint. | ||
| /// | ||
| /// @return Hint, as a types::bson_value::view. The caller must ensure that the returned object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// @return Hint, as a types::view. The caller must ensure that the returned object |
To match return type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated both code and docs to consistently use bsoncxx::v_noabi::types::view.
| /// | ||
| /// Returns a types::bson_value::view representing this hint. | ||
| /// | ||
| /// @return Hint, as a types::bson_value::view. The caller must ensure that the returned object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// @return Hint, as a types::view. The caller must ensure that the returned object |
To match return type.
Resolves CXX-3237 and CXX-3238 for the
v1::hintcomponent.To document/explain why
v_noabi::hintis not refactored in terms ofv1::hint(and similar classes with "transitive view accessors" in upcoming PRs): attempting to do so exposes violations of the "The caller must ensure that the returned object not outlive thehintobject that it was created from" contract for the.to_value()accessor in collection test code (which is nevertheless "fixed" by this PR, even though the behavior continues to be supported):bsoncxx::v_noabi::types::view t; { hint h{"abc"}; // Stored as a view. t = h.to_value(); // Copied as a view. } // `h` is destroyed, but the view of the "abc" string literal remains valid. CHECK(t == bsoncxx::v_noabi::types::b_string{"abc"}); // OK...? :(Despite being within our rights given the existing public API documentation to enforce this lifetime requirement, it is likely too risky to enforce this behavior and potentially break users (by undefined behavior!) who are (knowingly or unknowingly) taking advantage of this "view outlives the intermediate object" behavior (by Hyrum's Law). Therefore,
v_noabi::hintcan only supportv_noabi <-> v1conversions (e.g. as done forv_noabi::options::data_keyandv_noabi::options::tls). (Note: we could consider enforcing this postcondition by adding newk_invalid_<object>exceptions to the v_noabi API, but this is also likely to be an API breaking change too disruptive to justify in a non-major release.)This "transitive view accessor" backward compatibility problem also applies to other classes with both
view_or_valueaccessors (although onlyv_noabi::hintpublically documents that thehintobject must outlive the maybe-views returned by the class). Such classes will also only implementv_noabi <-> v1conversion support only without an implementation refactor.