Skip to content

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

Merged
merged 37 commits into from
Jan 4, 2024

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Dec 12, 2023

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
  • Redeclaration of v_noabi entities in the mongocxx root namespace via using-declarations.
  • Substitute mongocxx::X -> mongocxx::v_noabi::X in ABI namespace code (not in test code).
  • Removal of 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:

  • bsoncxx::stdx is redeclared in mongocxx::stdx and mongocxx::v_noabi::stdx separately, with mongocxx::v_noabi::stdx being marked as deprecated. This is to reflect the fact that mongocxx::stdx is just a convenient, straightforward redeclaration of bsoncxx::stdx entities. As with all other referenced bsoncxx entities, those redeclared in mongocxx::stdx are neither owned or managed by mongocxx. It's the bsoncxx library's responsibility to ensure ABI stability of the stdx entities being redeclared. Therefore, there is no reason to continue encouraging mongocxx::v_noabi::stdx: the entities declared within are not owned by mongocxx, thus have no direct relevance to the mongocxx library's ABI version. Retracted: CXX-2790 Redeclare mongocxx::v_noabi as a non-inline namespace #1070 (comment)
  • The error message strings in 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.
  • The mocked definition of MONGOCXX_LIBMONGOC_SYMBOL is now declared with MONGOCXX_TEST_API rather than MONGOCXX_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.

@eramongodb eramongodb self-assigned this Dec 12, 2023
@kevinAlbs
Copy link
Collaborator

If it is not important to preserve this behavior, the messages can be changed as well.

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 error_category is unlikely to meaningfully break consumer code (maybe could break tests?).

I slightly prefer to remove namespace from the error message. E.g. change invalid use of default constructed or moved-from mongocxx::client object to invalid use of default constructed or moved-from client object. If the ABI namespace is included in the message (e.g. mongocxx::v_noabi::client), I expect it may prevent reuse of the error codes in future ABI versions.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a 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 in rewrap_many_datakey.hpp
  • Replace remaining use of mongocxx::stdx::optional with bsoncxx::v_noabi::stdx::optional (Disregard: only used in private implementation)

@eramongodb
Copy link
Contributor Author

eramongodb commented Dec 14, 2023

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 v_noabi is not a stable ABI namespace, but there is no better time to test and establish patterns to use in the upcoming actual stable ABI namespaces than now.

Latest changes verified by this patch.

Copy link
Contributor

@vector-of-bool vector-of-bool left a 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.

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.

3 participants