Migrate project to C++17 and remove binary variant#102
Open
joshbainbridge wants to merge 2 commits into
Open
Conversation
In C++14, constexpr namespace-scope variables have internal linkage, so header-only installs duplicate the ~1.5 MB of blue noise tables in every translation unit that includes them. The binary install variant existed only to work around this, at the cost of three CMake options, conditional target logic, and a preprocessor split in bntables.h. C++17 inline variables have external linkage with a single definition across translation units, removing the need for the workaround. A benchmark recorded in the spec shows the C++17 header-only build matches the binary variant's size exactly, with identical output. Declare the blue noise tables as inline constexpr in bntables.h and delete src/bntables.cpp. Remove the OPENQMC_ENABLE_BINARY, OPENQMC_SHARED_LIB and OPENQMC_FORCE_PIC options, making the library target unconditionally INTERFACE with cxx_std_17. Raise the standard in the unix preset and the manual include example. Update the README, mkdocs home page and CHANGELOG for C++17, VFX Reference Platform CY2021 and NVCC 11. Record validation results in the migration spec, and use auto in trace.cpp for a modernize-use-auto finding that the standard bump newly triggers. Resolves #86 Signed-off-by: Josh Bainbridge <josh.bainbridge@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In C++14, constexpr namespace-scope variables have internal linkage,
so header-only installs duplicate the ~1.5 MB of blue noise tables in
every translation unit that includes them. The binary install variant
existed only to work around this, at the cost of three CMake options,
conditional target logic, and a preprocessor split in bntables.h. C++17
inline variables have external linkage with a single definition across
translation units, removing the need for the workaround. A benchmark
recorded in the spec shows the C++17 header-only build matches the
binary variant's size exactly, with identical output.
Declare the blue noise tables as inline constexpr in bntables.h
and delete src/bntables.cpp. Remove the OPENQMC_ENABLE_BINARY,
OPENQMC_SHARED_LIB and OPENQMC_FORCE_PIC options, making the library
target unconditionally INTERFACE with cxx_std_17. Raise the standard
in the unix preset and the manual include example. Update the README,
mkdocs home page and CHANGELOG for C++17, VFX Reference Platform CY2021
and NVCC 11. Record validation results in the migration spec, and use
auto in trace.cpp for a modernize-use-auto finding that the standard
bump newly triggers.
Resolves #86