Skip to content

CXX-2745 reorganize and tidy up internal code to support multiple ABI interfaces #1323

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 25 commits into from
Feb 12, 2025

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jan 30, 2025

An intermediate PR related to CXX-2745 following #1318.

Tip

Review by commit is recommended due to a large number of renamed files.

Tidies up and modernizes internal (private and test) code to better support multiple ABI interfaces.

Renames and relocates all private headers (under lib/) into a private/ subdirectory for consistent distinction from public and test headers (e.g. include directive sorting and grouping).

These components are also reused internally by the implementations of multiple ABI interfaces (does not contribute to or conflict with ABI stability), thus they do not belong under v_noabi/.

Notable changes include:

  • Renaming private/export.hh <- test_util/export_for_testing.hh.
    • Added a transitive include of v1/config/export.hpp to apply consistent component structure.
  • Renaming test_util/mock.hh <- private/mock.hh.
    • The test_util namespace remains as-is.
  • Removal of internal macro guard headers (prelude.hh and postlude.hh).
    • We do not need to worry about leaking internal macros into our own code.
    • Reduces and removes dependence on the old include-via-prelude pattern and takes advantage of new, consistent support for standalone inclusion.
  • Tidy up itoa.hh (signedness consistency, avoiding default arguments for exported functions).
  • Tidy up suppress_deprecation_warnings.hh (use new warning suppression macros).
  • Tidy up bson.hh (<- libbson.hh) (use new warning suppression macros).
    • Removed note regarding MSVC warnings (CXX-1366) as they no longer seem to apply.
    • Applies Level 1 warnings for the scope of the bson.h header via the warnings(push, 1) feature.
  • Tidy up mongoc.hh (<- libmongoc.hh) (use new warning suppression macros).
    • Use X-macro instead of X-macro-header to avoid non-standalone-includable headers. (This will also be done for the bsoncxx/enums/ X-macro-headers in an upcoming PR.)

@eramongodb eramongodb requested a review from kevinAlbs January 30, 2025 19:46
@eramongodb eramongodb self-assigned this Jan 30, 2025
@eramongodb eramongodb changed the title CXX-2745 Add advanced CMake options to toggle root namespace redeclarations CXX-2745 reorganize and tidy up internal code to support multiple ABI interfaces Jan 30, 2025
@eramongodb
Copy link
Contributor Author

PR description is fixed/updated.

@eramongodb
Copy link
Contributor Author

Apologies, identified some stray local dev changes which were not properly refactored prior to PR creation. Give me a minute to address them.

@eramongodb
Copy link
Contributor Author

Ready for review.


#else
#else // defined(MONGOCXX_TESTING) ^|v !defined(MONGOCXX_TESTING)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment fragment? I am not sure what ^|v refers to.

Copy link
Contributor Author

@eramongodb eramongodb Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indicating the #else is changing the "scope" of the preprocessor condition from defined(MONGOCXX_TESTING) above (^) to !defined(MONGOCXX_TESTING) below (v) such that trailing comments for both #if and #endif have a consistent counterpart (greppable) without being misleading about the condition (negated). Combined, ^|v is basically "☝️👇" ("see: above and below"). I can remove this style of comment if it is confusing.

I only just realized from your question that this style of comment is novel to our codebases. I'd thought it'd been committed before, but I seem to be mistaken. The only existing cases of this pattern are in the recently-committed postlude.cpp v1 implementation files.

// ^|v Prelude header must come before postlude header.

I do not remember where I picked up this pattern from. They may have used a different syntax than ^|v, which I vaguely recall being an abbreviation of what I originally saw, e.g.:

#else // ^^^ cond ^^^ | vvv !cond vvv

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thank you for the explanation. I slightly prefer the more verbose ^^^ cond ^^^ | vvv !cond vvv since I expect I would recognize it faster. But I am OK with the current style.

@eramongodb eramongodb requested a review from kevinAlbs February 4, 2025 17:44

#else
#else // defined(MONGOCXX_TESTING) ^|v !defined(MONGOCXX_TESTING)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thank you for the explanation. I slightly prefer the more verbose ^^^ cond ^^^ | vvv !cond vvv since I expect I would recognize it faster. But I am OK with the current style.

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.

LGTM. I appreciate the progressive flattening of the deep directory structure.

.clang-format Outdated
Comment on lines 163 to 167
- Regex: 'bsoncxx/config/prelude\.(hpp|hh)' # v_noabi preludes
Priority: 62
- Regex: 'mongocxx/config/prelude\.(hpp|hh)' # v_noabi preludes
Priority: 63
- Regex: 'bsoncxx/private/.*' # private headers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Can these YAML mappings be re-sorted by their Priority field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Priority: 6* categories look like they can be reordered, so updated as suggested.

However, in general, unfortunately no. The "first regex match" in the order they are listed is what determines the group and priority of the header. e.g. given:

- Regex: '<[[:alnum:]_.]+>' # system headers
  Priority: 30
- Regex: 'bsoncxx/private/.*' # private headers
  Priority: 60

the <bsoncxx/private/.*> headers will never match the priority 60 "private headers" regex because they'll always match the priority 30 "system headers" regex first.

Comment on lines 59 to 64
if(1)
configure_file(
bsoncxx/v_noabi/bsoncxx/config/private/config.hh.in
bsoncxx/v_noabi/bsoncxx/config/private/config.hh
bsoncxx/private/config/config.hh.in
bsoncxx/private/config/config.hh
)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside: What's up with the if(1) blocks all over the CMake code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a primitive workaround to make up for the lack of "scope blocks" in CMake without needing to resort to function() (overkill for these use cases) or comment-based begin+end delimiters. It is comparable to function-local block statements in C/C++ without any condition or loop.

That being said, this particular if(1) isn't doing anything useful anymore, so I've removed it in response.

Note: CMake 3.25 added block() which would be a much better and more appropriate command to use than if(1) once the minimum CMake required version is raised accordingly.

@eramongodb eramongodb merged commit 431ea20 into mongodb:master Feb 12, 2025
16 of 18 checks passed
@eramongodb eramongodb deleted the cxx-abi-internals branch February 12, 2025 20:32
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