-
Notifications
You must be signed in to change notification settings - Fork 543
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_count
and@see mongocxx::count_documents
in collection.hpp. - Add ABI namespace to
mongocxx::result::bulk_write
inrewrap_many_datakey.hpp
Replace remaining use of(Disregard: only used in private implementation)mongocxx::stdx::optional
withbsoncxx::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. |
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_point
in 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_noabi
v_noabi
entities in themongocxx
root namespace via using-declarations.mongocxx::X
->mongocxx::v_noabi::X
in ABI namespace code (not in test code).v_noabi::
for private and test entities (not specific to an ABI version).MONGOCXX_INLINE
removed 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::stdx
is redeclared inmongocxx::stdx
andmongocxx::v_noabi::stdx
separately, withmongocxx::v_noabi::stdx
being marked as deprecated. This is to reflect the fact thatmongocxx::stdx
is just a convenient, straightforward redeclaration ofbsoncxx::stdx
entities. As with all other referenced bsoncxx entities, those redeclared inmongocxx::stdx
are neither owned or managed by mongocxx. It's the bsoncxx library's responsibility to ensure ABI stability of thestdx
entities 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.cpp
are 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_SYMBOL
is now declared withMONGOCXX_TEST_API
rather thanMONGOCXX_API
to 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.