-
Notifications
You must be signed in to change notification settings - Fork 544
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
CXX-2745 relocate top-level dependencies into v1 headers #1318
base: master
Are you sure you want to change the base?
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.
Additional notes:
/// | ||
|
||
/// | ||
/// @dir bsoncxx/v1 |
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.
There is no bsoncxx/
subdirectory under v1/
(e.g. bsoncxx/v1/bsoncxx/
) as there is no future plans to update or remove the existing include directory paths to bsoncxx/v_noabi/bsoncxx/
to support <bsoncxx/header.hpp>
(unstable ABI interfaces). The v_noabi
headers will continue to provide root namespace redeclarations even after stable ABI interfaces are released and the ABI version number is made stable. Only users who need to maintain ABI stability requirements will need to include ABI-specific headers (e.g. following an API breaking change that updates redeclarations from v1 -> v2
while preserving the ABI stability of v1
symbols).
BSONCXX_IF_GCC | ||
BSONCXX_IF_CLANG | ||
BSONCXX_IF_GNU_LIKE | ||
BSONCXX_RETURNS | ||
INCLUDE_PATTERNS | ||
"include/*.hpp" # Public headers. |
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.
Reminder that the macro guard tests do not test generated headers (config.hpp, export.hpp, version.hpp), thus their provision of macros when standalone-included is not a concern (as is the case with macros.hpp
).
INCLUDE_PATTERNS | ||
"include/*.hpp" # Public headers. | ||
EXCLUDE_REGEXES | ||
"include/bsoncxx/docs/.*\.hpp" # Doc header. | ||
"include/.*/(prelude|postlude)\.hpp" # Macro guard headers. | ||
"include/bsoncxx/v_noabi/bsoncxx/config/.*\.hpp" # v_noabi config headers. | ||
"include/bsoncxx/v1/detail/macros\.hpp" # v1 private macros. |
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.
macros.hpp
must be excluded due to deliberately providing guarded macros when standalone-included.
INCLUDE_PATTERNS | ||
"include/*.hpp" # Public headers. | ||
EXCLUDE_REGEXES | ||
"include/bsoncxx/docs/.*\.hpp" # Doc header. | ||
"include/.*/(prelude|postlude)\.hpp" # Macro guard headers. | ||
"include/bsoncxx/v_noabi/bsoncxx/config/.*\.hpp" # v_noabi config headers. | ||
"include/bsoncxx/v1/detail/macros\.hpp" # v1 private macros. | ||
"include/bsoncxx/v_noabi/bsoncxx/config/.*\.hpp" # v_noabi include-via-prelude headers. |
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.
"Include-via-prelude" headers remain excluded as their guarded macros continue to be provided via prelude.hpp
instead. Even if they are now standalone-includable, updating existing code which obtains guarded macros via prelude.hpp
is low-priority..
|
||
#pragma push_macro("BSONCXX_PRIVATE_V1_INSIDE_MACRO_GUARD_SCOPE") | ||
#undef BSONCXX_PRIVATE_V1_INSIDE_MACRO_GUARD_SCOPE | ||
#define BSONCXX_PRIVATE_V1_INSIDE_MACRO_GUARD_SCOPE |
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.
A dedicated scope macro can be used to track and enforce correct 1-to-1 inclusion of the prelude and postlude headers fron their initiation, unlike with v_noabi macro guard headers (CXX-2770 + continued backward compatibility with the v_noabi include-via-prelude pattern).
#cmakedefine BSONCXX_POLY_USE_IMPLS | ||
#cmakedefine BSONCXX_POLY_USE_STD | ||
|
||
/// | ||
/// @file | ||
/// Provides macros describing the bsoncxx library configuration. | ||
/// | ||
/// @warning This header is not standalone includable! |
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.
This warning is no longer applicable due to the v1 macro guard strategy and backward compatibility with the v_noabi include-via-prelude pattern. Users are now free to include config.hpp
and version.hpp
directly (even via v_noabi
headers).
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.
This header file remains for forward-compatibility with the introduction of configuration options for the mongocxx library (which are independent of the bsoncxx library configuration). If this possibility seems unlikely, we may consider removing this header entirely (for both v1 and v_noabi).
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.
No strong opinion. No objection to keeping for forward-compatibility.
Deferred the changes in 9096a30 (relocation of internal components out of v_noabi) to a followup PR to simplify review for this PR. |
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.
No strong opinion. No objection to keeping for forward-compatibility.
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.
The thorough documentation in this PR, efforts to make headers standalone includable, and better organization (movement of internal API into details
) is greatly appreciated. I have a question that might lead to simplification and can review again if needed. Otherwise, the current PR LGTM.
Summary
This is 1 out of an estimated 7 major PRs which in total are expected to resolve CXX-2745.
(This estimate does not account for additional minor PRs which may be interspersed throughout and alongside these planned major PRs.)
This is the long-overdue successor to the initial attempt to start migrating interfaces to v1 by #1052.
Due to the volume of changes required by this ticket, the PRs are planned to be split for review as follows:
Upcoming PRs will be rebased on top of changes applied to this PR during review, as well as other commits applied to the repository in the meanwhile.
Note
This PR is organized in a manner that encourages commit-by-commit review. Relocations of files or components are given dedicated commits labeled:
v1 <- v_noabi: <name>
. All subsequent modifications to relocated (or new) files or components and related changes are grouped into commits labeled:v1: <name>
. Therefore, builds are broken forv1 <- v_noabi: <name>
commits; all other commits preserve build validity.Relative Changelog (v_noabi -> v1)
This section documents changes applied to v1 interfaces relative to their prior equivalent in v_noabi (when applicable). This is not a changelog for
v_noabi
interfaces. This is to help keep track of the changes that are required to upgrade from v_noabi to v1.Additional Features
These are new features in v1 which did not previously exist in v_noabi:
BSONCXX_VERSION_STRING
: for symmetry withMONGOCXX_VERSION_STRING
.Changed Features
These are features which have changed behavior in v1 relative to their equivalent in v_noabi:
prelude.hpp
andpostlude.hpp
are moved fromconfig/
todetail/
.stdx/operators.hpp
is renamed todetail/compare.hpp
(a la<compare>
).Removed Features
These are features which have been removed in v1, constituting a deliberate breaking change for code migrating from v_noabi:
config/compiler.hpp
(v_noabi) is removed (superceded bydetail/macros.hpp
(v1)).config/util.hpp
(v_noabi) is removed (superceded bydetail/macros.hpp
(v1)).Absolute Changelog (v_noabi)
This section documents changes applied to v_noabi interfaces relative to the latest minor release. This is to help keep track of the changes which may affect upcoming releases.
Additional Features
BSONCXX_VERSION_STRING
: for symmetry withMONGOCXX_VERSION_STRING
.Changed Features
optional<T>
andstring_view
underbsoncxx::stdx
andbsoncxx::v_noabi::stdx
are now redeclarations ofbsoncxx::v1::stdx
.stdx
interfaces.Removed Features
Detailed Description
This section elaborates in detail the changes in this PR, including design decisions and design principles:
Dependencies Between v1 and v_noabi
The relationship between stable and unstable interfaces is comparable to
const
vs. non-const
:S::v1() const
cannot invokeS::v_noabi()
(non-const).S::v_noabi()
(non-const) can invokeS::v1() const
.This restriction does not apply to the implementations of said interfaces, where a
v1
source file may include av_noabi
(internal) header file. v1 implementations may use v_noabi interfaces, but only those which are NOT exported (e.g. internal/private helpers) so that they do not transitively inherit ABI (in)stability. ABI v1 stability must not transitively depend on v_noabi stability.Therefore, to better distinguish v_noabi interfaces from internal (helper) interfaces, all components under(Deferred to a followup PR.)private/
andtest_util/
have been relocated out ofv_noabi/
into their own subdirectories.This
v1 <- v_noabi
("v_noabi depends on v1") principle will be preserved for all upcoming PRs.Important
Polyfills in
stdx
MUST be associated with thev1
namespace, as their use in v1 interfaces tightly associates their definition (givenBSON_POLY_USE_IMPLS=ON
) with the stability of the v1 ABI. In the future, there is a high incentive to correlate migration fromv1 -> v2
(stable ABI breaking change) with a release that drops support for pre-C++17 standards and theBSON_POLY_USE_IMPLS=ON
configuration (raising the minimum required C++ standard to C++17). To this end, when introduced,v2
interfaces should require C++17 or newer.Clarity of Public vs. Internal-Use Interfaces
Due to how v_noabi macro guard headers are designed, headers under
config/
have over time expanded to include "detail" interfaces (or "internal", or "implementation") such ascompiler.hpp
andutil.hpp
despite having no (longer any) relevance to library configuration. Similarly, headers understdx/
have expanded to provide type traits, utilities, and other helpers designed for internal usage only (such as the implementation of polyfills) despite not being part of our public API.In all cases, installed headers providing top-level, library-wide "for internal use only" interfaces are now separated from the public API under the
detail/
directory, aligning with their declaration within thedetail
namespace when applicable. Similarly, headers underconfig/
are now exclusively those which are generated by CMake during library configuration (config.hpp
,export.hpp
, andversion.hpp
).Macros are similarly relocated to more appropriate locations (such as
macros.hpp
) and consistently prefixed withBSONCXX_PRIVATE_*
orMONGOCXX_PRIVATE_*
to clearly document their "for internal use only" purpose (even when documented for explanatory purposes, e.g. as is the case for export macros). This consistency should make clearer to users which macros are reserved for internal use only.Note
Unlike with
private/
andtest_util/
, this PR does not movedetail/
out of ABI namespace directories into the root directory. This is to avoid any further confusion regarding relative include directory paths (wherebsoncxx/foo.hpp
is equivalent tobsoncxx/v_noabi/bsoncxx/foo.hpp
) and to avoid confusing the dependency relationships between ABI headers (e.g. thev1
<-v_noabi
relationship described above).Transitive Header Inclusion
Transitive includes are now documented by a
@par Includes
in API documentation.This will become an increasingly important feature as unstable ABI headers eventually provide stable ABI interfaces via using-declarations in root namespaces (e.g.
bsoncxx::document::view
as an alias forbsoncxx::v1::document::view
necessarily implies<bsoncxx/v1/document/view.hpp>
is provided). This will additionally facilitate easier adoption/transition fromv_noabi
tov1
interfaces when we eventually deprecate and remove v_noabi interfaces (root namespace redeclarations will continue to be provided byv_noabi
headers even after currentv_noabi
interfaces are removed).Note
Doxygen option
SHOW_INCLUDE_FILES
was deliberately set toNO
in #1166 to permit this selective documentation of transitive header includes.Standalone Includability
This is undoubtedly the most difficult-to-explain change in this PR. Please let me know if anything needs further clarification.
The v_noabi
config/
headers suffer from an inconsistent lack of standalone includability, forcing valuable public headers such asconfig.hpp
(informing the user about the library configuration) andversion.hpp
(inform the user about the library version) to be included alongside internal headers (e.g.compiler.hpp
,util.hpp
, etc.) via the macro guard prelude headerprelude.hpp
("include-via-prelude"), which should really be for internal use only.This PR ensures all headers (with the exception of the macro guard headers themselves) are standalone includable. This required some adjustments to how macro guards are defined in the macro guard headers. To support both standalone inclusion of these headers in v1, but also continuing to support the include-via-prelude pattern for backward compatibility, traditional macro guards are required to detect when an include-via-prelude header has already been included to avoid double-popping provided macros in the v_noabi postlude header.
Important
The ClangFormat configuration is updated such that v1 headers are always included before v_noabi headers to enforce the
v1 <- v_noabi
relationship.Using
export.hpp
(v1) as an example, when it is included in a v1 public header (note: the v1 prelude header comes before all other headers, unlike the v_noabi prelude header which comes after all other headers):When it is included in a v_noabi public header via the prelude header, it is identical to the above, but with an inner layer of v_noabi macro guards (note: the v_noabi prelude header comes after other v_noabi headers):
When it is included directly by a v_noabi public header, the provided macros will be made available regardless of v_noabi macro guards (this is deliberate for consistency with behavior as a standalone-includable header):
This achieves backward compatibility with the former include-via-prelude-only behavior of v_noabi headers while supporting standalone inclusion as v1 headers.