Skip to content

CXX-2288 Add forward headers for bsoncxx and mongocxx #1061

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 68 commits into from
Dec 1, 2023

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Nov 10, 2023

Description

Resolves CXX-2288. Verified by this patch.

This PR initially proposes maximizing modularity by providing a forward header per-component. If this is considered excessive, forward headers can be merged as requested, i.e. merging mongocxx/events/(.*)-fwd.hpp into mongocxx/events/fwd.hpp.

Requesting reviews keep a close eye on whether BSONCXX_API and MONGOCXX_API are correctly preserved when moved into forward headers, and that they are not accidentally added to classes that did not have them prior.

Principles

There are several principles/rules that motivated/directed how changes in this PR were made. These are:

  • Any public header a.hpp that contains a declaration of a scoped enumeration or class type in namespace scope is given a forward header a-fwd.hpp containing a forward declaration of that enumeration or class type.
    • Public headers containing only declarations of deprecated entities are excluded. This only applies to mongocxx/options/create_collection.hpp.
  • A forward header a-fwd.hpp is always included by its corresponding a.hpp. This is both to physically encode the header dependency and to ensure the definition for class types in a.hpp is always consistent with its forward declaration in a-fwd.hpp.
  • Symbol visibility for class types using (BSONCXX|MONGOCXX)_API is always declared in the forward
    header (when applicable). This guarantees consistent symbol visibility regardless whether the class declaration is imported via the forward or normal header. (A followup PR may relocate Doxygen documentation comments for enumeration/class types into the forward header.) The only instances of (BSONCXX|MONGOCXX)_API for class declarations in normal headers is now limited to member classes and deprecated classes.
  • All friend declarations are fully qualified relative to the global scope to minimize any potential for confusion about what class is being declared as a friend. The addition of forward headers that include these changes are made in separate commits in this PR to make it easier to track the related changes. (Unfortunately, C++ does not provide a mechanism that enforces a friend declaration always refers to an existing declaration, rather than introducing a new declaration.)
  • Although using-declarations (using foo::bar;), using-directives (using namespace foo;), type aliases (using foo = bar;), and typedefs in namespace scope could be moved into forward headers, this was avoided to keep the forward headers as simple as possible. Therefore, the forward headers contain no using or typedef declarations. Headers that only declare a type alias or typedef are not given a forward header due to redundancy, e.g. bsoncxx/document/view_or_value.hpp.

Additionally:

  • For convenience (and as requested by CXX-2288), a forward header for all bsoncxx/mongocxx entities is provided as bsoncxx/fwd.hpp and mongocxx/fwd.hpp. These headers simply include all the (.*)-fwd.hpp headers.
  • Ensured all class declarations have only a single definitive declaration that specifies symbol visibility. This only applied to member classes such as bsoncxx::document::view::iterator.
  • Reduced potential confusion (both for C++ name lookup and simple readability) when referring to (un/under)qualified names, e.g. bsoncxx::document vs. bsoncxx::builder::document.

ClangFormat

The forward headers, identified as either .../fwd.hpp or .../<name>-fwd.hpp, are prioritized before normal headers. Ideally, system headers should be deprioritized to come after all normal headers (including the macro guard headers), but that change is deferred to avoid increasing the already large PR diff. The header sort is thus the following:

#pragma once

#include <system> // System Headers

#include <bsoncxx/document/value-fwd.hpp> // Forward Headers (new!)

#include <bsoncxx/document/view.hpp> // Normal Headers

#include <bsoncxx/config/prelude.hpp> // Macro Guard Header

// ... (normal header contents)

#include <bsoncxx/config/postlude.hpp> // Macro Guard Header

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.

LGTM with suggestion: update the Headers section of CONTRIBUTING.md to match the new expected header order. I had not considered CONTRIBUTING.md when reviewing related ABI PRs.

Requesting reviews keep a close eye on whether BSONCXX_API and MONGOCXX_API are correctly preserved when moved into forward headers, and that they are not accidentally added to classes that did not have them prior.

LGTM. I assume classes like bsoncxx::v_noabi::bsoncxx::view_or_value did not, and do not include BSONCXX_API due to methods being inlined.

@eramongodb
Copy link
Contributor Author

eramongodb commented Nov 15, 2023

I assume classes like bsoncxx::v_noabi::bsoncxx::view_or_value did not, and do not include BSONCXX_API due to methods being inlined.

Correct. The view_or_value class template (and related aliases) do not require export macros due to all related functions being declared BSONCXX_INLINE (AFAIK redundantly, given we compile with hidden visibility by default for shared libraries). In other words, the related functions of view_or_value<View, Value> are part of the public API but not part of the public ABI, even when View and/or Value are part of the public ABI. I think (intentionally or not) this is consistent with their intended usage as "forwarding" parameters for view and value types. (Note: the layout of data members of view_or_value is still unavoidably part of the ABI due to its use by exported symbols, even if the related functions are not.)

@eramongodb
Copy link
Contributor Author

Identified more instances of (re)declarations of class types in the form of "elaborated type specifiers". Replaced all such instances with references to the already-declared class type instead, to ensure the use of (forward) headers containing the single definitive declaration for said class type. The sole exceptions are the definitions of the static local category variables in the error_category() and server_error_category() functions to disambiguate the class types with the same name as the functions they are used in.

Latest changes verified by this patch.

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.

4 participants