-
Notifications
You must be signed in to change notification settings - Fork 543
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
Conversation
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.
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
andMONGOCXX_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.
Correct. The |
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 Latest changes verified by this patch. |
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
intomongocxx/events/fwd.hpp
.Requesting reviews keep a close eye on whether
BSONCXX_API
andMONGOCXX_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:
a.hpp
that contains a declaration of a scoped enumeration or class type in namespace scope is given a forward headera-fwd.hpp
containing a forward declaration of that enumeration or class type.mongocxx/options/create_collection.hpp
.a-fwd.hpp
is always included by its correspondinga.hpp
. This is both to physically encode the header dependency and to ensure the definition for class types ina.hpp
is always consistent with its forward declaration ina-fwd.hpp
.(BSONCXX|MONGOCXX)_API
is always declared in the forwardheader (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.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 nousing
ortypedef
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:
bsoncxx/fwd.hpp
andmongocxx/fwd.hpp
. These headers simply include all the(.*)-fwd.hpp
headers.bsoncxx::document::view::iterator
.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: