-
Notifications
You must be signed in to change notification settings - Fork 547
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
kevinAlbs
left a comment
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_APIandMONGOCXX_APIare 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.hppintomongocxx/events/fwd.hpp.Requesting reviews keep a close eye on whether
BSONCXX_APIandMONGOCXX_APIare 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.hppthat contains a declaration of a scoped enumeration or class type in namespace scope is given a forward headera-fwd.hppcontaining a forward declaration of that enumeration or class type.mongocxx/options/create_collection.hpp.a-fwd.hppis always included by its correspondinga.hpp. This is both to physically encode the header dependency and to ensure the definition for class types ina.hppis always consistent with its forward declaration ina-fwd.hpp.(BSONCXX|MONGOCXX)_APIis 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)_APIfor 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 nousingortypedefdeclarations. 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.hppandmongocxx/fwd.hpp. These headers simply include all the(.*)-fwd.hppheaders.bsoncxx::document::view::iterator.bsoncxx::documentvs.bsoncxx::builder::document.ClangFormat
The forward headers, identified as either
.../fwd.hppor.../<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: