Repository navigation
Use an inline constexpr for implicit inlining - #22
Merged
Merged
Conversation
AX_CXX_COMPILE_STDCXX was inside the AS_IF that guards the gtest checks, so --without-gtest asked for no particular standard at all. The library headers need C++17 whether or not we build the unit tests, so run the check unconditionally. AC_LANG stays behind in the gtest block: AX_CXX_COMPILE_STDCXX does its own AC_LANG_PUSH([C++]), but AC_CHECK_HEADER([gtest/gtest.h]) and the AC_LINK_IFELSE that follows it do need the C++ compiler selected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
At namespace scope, static const gives internal linkage, so every translation unit including memtailor.h got its own private copy of MemoryAlignment and BitsPerByte. The inline functions in MemoryBlocks.h name both of them, which is only well defined thanks to the carve-out in [basic.def.odr] for const objects of literal type that hold the same value in every definition. Since C++17, inline constexpr makes each of these a single entity across the whole program, so we no longer lean on that carve-out. Note that plain constexpr would not have been enough: at namespace scope it implies const and hence internal linkage, so inline is the part doing the work here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both build systems ask for C++17, but that says nothing about how the installed headers get compiled downstream. They use an inline variable, so give users of the library a clear diagnostic rather than whatever the compiler makes of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
A follow-up to Macaulay2/mathic#25 and Macaulay2/mathicgb#62, though the
conclusion here is different.
memtailor is not vulnerable to the undefined-reference bug from those PRs
(Debian #1145665). That one needs a class-scope
static constwith an in-classinitializer, which is only a declaration and needs an out-of-line definition
once odr-used. memtailor has none. Its only two constants are at namespace
scope, where
static constis a definition in every translation unit, so nolinker error is possible. Nothing here is broken today.
What the sweep did turn up:
Namespace-scope
staticmeans internal linkage, so every TU includingmemtailor.hgot its own private copy ofMemoryAlignmentandBitsPerByte.The inline functions in
MemoryBlocks.hname both, which is well defined onlythanks to the carve-out in [basic.def.odr] for const objects of literal type
holding the same value in every definition.
inline constexprmakes each asingle entity and drops the reliance on that carve-out. Note that plain
constexprwould not have been enough: at namespace scope it impliesconstand hence internal linkage, so
inlineis the part doing the work.AX_CXX_COMPILE_STDCXXsat inside theAS_IFguarding the gtest checks, so--without-gtestasked for no particular standard at all. The headers needC++17 whether or not we build the unit tests.
AC_LANGstays behind in thegtest block, since
AX_CXX_COMPILE_STDCXXdoes its ownAC_LANG_PUSH([C++])but
AC_CHECK_HEADER([gtest/gtest.h])does need the C++ compiler selected.The
#errorguard then gives downstream users a clear diagnostic, which mattersnow that the headers contain an inline variable.
Tested:
make distcheck, andmake checkunder both--with-gtestand--without-gtestfrom a clean out-of-tree build; cmake builds with 26/26 testspassing. Confirmed the guard fires at
-std=c++14, and that under a forcedodr-use at
-O0nmnow reports a single unique global symbol for eachconstant rather than a per-TU local.
One caveat worth flagging: the toolchain I tested on has gtest 1.14, so these
runs are not evidence about gtest 1.18 specifically. The absence of class-scope
statics is the reason to expect memtailor is clear there, not the test runs.
Claude Opus 5 did the analysis and drafted the changes.