Skip to content

CXX-2745 Migrate library config headers into v1 #1052

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

Closed
wants to merge 9 commits into from

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Nov 6, 2023

Description

This PR is the first of many related to CXX-2745. Verified by this patch.

This PR is the first to add stable ABI headers to the CXX Driver. 🎉

This PR does/should NOT break source or binary compatibility. This PR also serves as a template for the rest of the upcoming PRs that will be migrating contents of v_noabi headers into v1 headers.

Stable ABI Documentation

The updates to documentation deserve special attention.

To quote Ulrich Drepper:

The definition of stability should therefore use the documented interface as the basis.

And to quote the SemVer spec:

For this system to work, you first need to declare a public API. This may consist of documentation or be enforced by the code itself. Regardless, it is important that this API be clear and precise.

Therefore, this PR and those upcoming that are related to CXX-2745 take great care to ensure the documentation for migrated entities is of sufficient quality for the purposes of defining and maintaining a stable ABI. There should be no confusion as to what is considered a part of the public API or ABI, and what entities are guaranteed stability or not.

First and foremost, the SHOW_INCLUDE_FILES Doxygen config variable is changed to NO. We do not want to encourage users to depend on circumstantial transitive header inclusion. Setting this variable to NO allows us to be explicit about when we do deliberately support transitive header inclusion. This is done via the use of @par Includes + @li of included headers.

Second, Doxygen complains about documentation of preprocessor macros for which it cannot find a #define directive. This applies to all macros defined in generated headers. Therefore, the prelude header uses the (BSONCXX|MONGOCXX)_SATISFY_DOXYGEN_DEFINES macro to toggle the definition of "dummy" #define directives solely for the purpose of satisfying the Doxygen parser. Although it would be nice for the documentation to be in their corresponding generated headers, this would significantly complicate the steps necessary to generate documentation and so was avoided. Doxygen "groups" is also used to dedicate a "Topic" page for each generated header file for associativity and ease of reference of generated macros.

Third, as done in #1039, Doxygen documentation for global namespaces is added/moved into the v1 prelude header. Documentation pertaining to v_noabi namespaces is left in the v_noabi prelude header. The list of such documentation will continue to expand as more headers are migrated to v1.

In total, this is to ensure clear and precise documentation concerning the meaning and use of config macros which are (in v_noabi), and will be (in v1), used to define the public API/ABI. These macros are currently completely undocumented despite their significance. This is especially important for the macros defined by export.hpp and in compiler.hpp (i.e. BSONCXX_INLINE).

Config Header Migration

The pattern used for the migration of config headers is as follows:

  • Contents of the v_noabi header is moved directly into the v1 header.
  • The v_noabi header includes the v1 header.
  • Contents that are specific to the v_noabi header remain in the v_noabi header (i.e. *_INLINE_NAMESPACE_* macros).

This effort is necessary to ensure that no stable ABI header depends on an unstable ABI header. Fundamentally, the stable ABI should not depend on the unstable ABI. This is structurally enforced by stable ABI headers never including unstable ABI headers (there is no test/check for this, but should be strongly enforced via code review). On the other hand, it is perfectly fine for the unstable ABI to depend on the stable ABI. Therefore, the v_noabi config headers are free to include and reuse definitions in the v1 config headers. This pattern will be reflected by all stable ABI headers moving forward.

The v1 macro guard headers (prelude.hpp and postlude.hpp) are given a macro guard validity check in the form of the *_V1_INSIDE_MACRO_GUARD_SCOPE macro. This is to avoid further instances of CXX-2770 (missing/mismatched macro guard header includes) in v1 headers moving forward.

The addition of .cpp files for each migrated header file is to ensure backward compatibility (macros
currently defined by a v_noabi header is still defined by the v_noabi header) and also ensure standalone header inclusion support. These may be removed if deemed excessive/unnecessary.

@eramongodb eramongodb self-assigned this Nov 6, 2023
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.

I like the use of the INSIDE_MACRO_GUARD_SCOPE to enforce expectations of including prelude.hpp:

// foo_error.hpp incorrectly includes prelude twice before a postlude.
#pragma once
#include <bsoncxx/v1/bsoncxx/config/prelude.hpp>
// bar.hpp also includes the prelude.
#include <bsoncxx/v1/bsoncxx/bar.hpp> // Error due to macro guard.
BSONCXX_API void BSONCXX_CALL foo();
#include <bsoncxx/v1/bsoncxx/config/postlude.hpp>

The addition of .cpp files for each migrated header file is to ensure backward compatibility (macros currently defined by a v_noabi header is still defined by the v_noabi header) and also ensure standalone header inclusion support. These may be removed if deemed excessive/unnecessary.

No strong preference. I expect an ABI compliance task would similarly catch if the macros are unintentionally removed. But I do not think the .cpp files add much maintenance burden.

Copy link
Contributor

@kkloberdanz kkloberdanz 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! Great job!

@eramongodb
Copy link
Contributor Author

Will reopen a new PR with rebased changes once all blockers have been addressed (see CXX-2745).

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