-
Notifications
You must be signed in to change notification settings - Fork 548
CXX-2790 Redeclare mongocxx::v_noabi as a non-inline namespace #1070
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
I expect there are no documented guarantees to preserve error messages. Though it would not surprise me if some error messages are relied on by consumers. Example: SERVER-41365 describes how the "ns not found" error message was relied on in drivers. I expect changing the namespace in the error messages of I slightly prefer to remove namespace from the error message. E.g. change |
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.
Looks good with additional suggested changes:
- Add ABI namespace to
@see mongocxx::estimated_document_countand@see mongocxx::count_documentsin collection.hpp. - Add ABI namespace to
mongocxx::result::bulk_writeinrewrap_many_datakey.hpp Replace remaining use of(Disregard: only used in private implementation)mongocxx::stdx::optionalwithbsoncxx::v_noabi::stdx::optional
|
Further discussion following feedback given above revealed references to bsoncxx entities in mongocxx should include an ABI namespace qualifier (this should have been done in #1066). This is to ensure ABI stability of the mongocxx library when the bsoncxx library changes the ABI version of a root namespace redeclaration, which shouldn't be an ABI breaking change for either the bsoncxx or mongocxx library. This also demonstrates correct, deliberate use of ABI namespace qualifiers in code that is designed for ABI stability (model for downstream users). This is all precluding the fact that Latest changes verified by this patch. |
vector-of-bool
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.
Apologies for the long delay. LGTM.
Brief
Followup to #1066. Resolves CXX-2790. Verified by this patch.
As before, this PR SHOULD NOT break source or binary compatibility. Verified by this patch. No exceptions this time, although the low severity warning for
system_clock::time_pointin the source compatibility report is curious.Description
Analogous to the changes in #1066 for the bsoncxx library, this PR applies the following changes to the mongocxx library:
inline namespace v_noabi->namespace v_noabiv_noabientities in themongocxxroot namespace via using-declarations.mongocxx::X->mongocxx::v_noabi::Xin ABI namespace code (not in test code).v_noabi::for private and test entities (not specific to an ABI version).MONGOCXX_INLINEremoved from private and test files (do not document public API).There are only two notable exceptions to the regular modification patterns above:
Retracted: CXX-2790 Redeclare mongocxx::v_noabi as a non-inline namespace #1070 (comment)bsoncxx::stdxis redeclared inmongocxx::stdxandmongocxx::v_noabi::stdxseparately, withmongocxx::v_noabi::stdxbeing marked as deprecated. This is to reflect the fact thatmongocxx::stdxis just a convenient, straightforward redeclaration ofbsoncxx::stdxentities. As with all other referenced bsoncxx entities, those redeclared inmongocxx::stdxare neither owned or managed by mongocxx. It's the bsoncxx library's responsibility to ensure ABI stability of thestdxentities being redeclared. Therefore, there is no reason to continue encouragingmongocxx::v_noabi::stdx: the entities declared within are not owned bymongocxx, thus have no direct relevance to the mongocxx library's ABI version.mongocxx/exception/error_code.cppare left unchanged despite referencing root namespace declarations without ABI namespace (e.g.mongocxx::client) to preserve existing behavior. If it is not important to preserve this behavior, the messages can be changed as well.MONGOCXX_LIBMONGOC_SYMBOLis now declared withMONGOCXX_TEST_APIrather thanMONGOCXX_APIto better reflect it's intended usage.Despite the greater diff volume due to more source code in mongocxx, the PR should otherwise be very similar to #1066.