benchmark
benchmark copied to clipboard
google
[BUG] requirement of C++11 is incompatible with Google Test
Google Benchmark insists on being compiled with the standard not exceeding C++11 https://github.com/google/benchmark/blob/974cd5a5c5a78e76ebc50961f4dbf3bf6d4ade4e/CMakeLists.txt#L174
At the same time, Google Benchmark requires Google Test, which is incompatible with C++11 https://github.com/google/googletest/blob/5b909beeec178f338be997830b6c31a80cda7a93/googletest/include/gtest/internal/gtest-internal.h#L635
typedef ::std::map<std::string, CodeLocation, std::less<>> RegisteredTestsMap;
std::less without template parameters doesn't compile in C++11.
this is somewhat surprising given all our CI bots pass on multiple platforms with multiple compilers.
what OS and compiler are you on?
My guess would be: your CI bots don't use BENCHMARK_USE_BUNDLED_GTEST=OFF. Ubuntu 16, Clang 14 with Clang's libc++. However, the OS is irrelevant, I build the whole toolset myself, it's OS-independent.
Also... let's not doubt the facts: less<> and -std=c++11 are incompatible. CI bots not finding it doesn't change this fact.
https://en.cppreference.com/w/cpp/utility/functional/less
sure, but then shouldn't you be building the GTEST bundled to make sure it's using the same ABI throughout the toolset?
I am building Gtest, with the toolset, not with Google Benchmark. Look, I hope this thread is not about my virtues, i.e. what I should or shouldn't do. I also hope it's not a version of "it works on my machine". I also don't need a workaround, I can patch the build files myself.
There is a crystal clear fact: std::less<> and -std=c++11 are incompatible. Pointing this out was my help to you. What you do with this fact is up to you. I don't need you telling me what I should or shouldn't do, I didn't ask for help in building my toolset. I merely tried to help you.
I am building Gtest, with the toolset, not with Google Benchmark. Look, I hope this thread is not about my virtues, i.e. what I should or shouldn't do. I also hope it's not a version of "it works on my machine". I also don't need a workaround, I can patch the build files myself.
that was not my intent at all. i'm trying to figure out why you have a toolchain that builds googletest with one setup (not c++11) and benchmark with another (it requires c++11 to build). i think it's unsurprising that if you do this, it's not going to work.
There is a crystal clear fact:
std::less<>and-std=c++11are incompatible. Pointing this out was my help to you. What you do with this fact is up to you. I don't need you telling me what I should or shouldn't do, I didn't ask for help in building my toolset. I merely tried to help you.
agreed. but i'm still confused about something: you say google benchmark is incompatible with C++11 (and yes, the documentation agrees) yet we build google test as part of google benchmark with c++11 set, and it works. so i don't understand where the gap is, or what to do about it:
- we can't change googletest to require c++11
- we can't remove the c++11 requirement from googlebenchmark
so as long as googletest can be built under c++11 (and i think us bundling it means that it can be) then that seems like the obvious solution: if anyone wants to use googletest with googlebenchmark, which requires c++11, they need to build googletest under c++11.
what am i missing? (this is not passive aggressive: i don't know what i'm missing)
why you have a toolchain that builds googletest with one setup (not c++11) and benchmark with another (it requires c++11 to build). i think it's unsurprising that if you do this, it's not going to work.
Because GTest requires at least C++14 (since it uses std::less<>), and Google Benchmark specify explicitly C++11 in the build files.
you say google benchmark is incompatible with C++11
I've never said that.
we build google test as part of google benchmark with c++11 set, and it works
If it works, then due to some bug because in C++11 std::less must have explicit template parameters. See: https://en.cppreference.com/w/cpp/utility/functional/less
so as long as googletest can be built under c++11
Only with a bug in the compiler. It, obviously, doesn't build in Clang 14 with -std=c++11.
why you have a toolchain that builds googletest with one setup (not c++11) and benchmark with another (it requires c++11 to build). i think it's unsurprising that if you do this, it's not going to work.
Because GTest requires at least C++14 (since it uses
std::less<>), and Google Benchmark specify explicitly C++11 in the build files.
ah, gotcha. thanks, that's what i was missing. so we should instead require at least C++11 rather than precisely C++11.
you say google benchmark is incompatible with C++11
I've never said that.
apologies, i meant google test.
we build google test as part of google benchmark with c++11 set, and it works
If it works, then due to some bug because in C++11
std::lessmust have explicit template parameters. See: https://en.cppreference.com/w/cpp/utility/functional/less
it works, as in, we're able to build googletest as part of the google benchmark project on our CI bots, but i don't know why this would work.. as in, our compilers we test on might all support C++14 but we're still setting the google benchmark (and implicitly google test) to C++11 which should indeed fail.
so as long as googletest can be built under c++11
Only with a bug in the compiler. It, obviously, doesn't build in Clang 14 with
-std=c++11.
i think this may be related to #1462: an issue with clang 15 on Windows.
it works, as in, we're able to build googletest as part of the google benchmark project on our CI bots
I never questioned that your CI bots work. It's irrelevant.
So we should instead require at least C++11 rather than precisely C++11.
As far as I know, you can't do it with command line options. IMHO the right solution is to remove
add_cxx_compiler_flag(-std=c++11)
altogether. If you decide to leave it, at least use CMake's CXX_STANDARD (https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html).
Does google test specify the minimal required C++ standard in it's CMake Interface?
today we (benchmark) set c++11 and fallback to c++0x. there's even a comment that c++14 doesn't work for some configurations.
ie, if we set CMAKE_CXX_STANDARD, which would be reasonable, it would potentially fail for c++14. however, in that case, i think we can consider it WAI in that the platform on which c++14 doesn't work can set CMAKE_CXX_STANDARD to what they need it to be.
so setting the default to C++11 seems reasonable.
@LebedevRI the googletest readme says:
#### C++ Standard Version
An environment that supports C++11 is required in order to successfully build
GoogleTest. One way to ensure this is to specify the standard in the top-level
project, for example by using the `set(CMAKE_CXX_STANDARD 11)` command.
If this is not feasible, for example in a C project using GoogleTest for validation,
then it can be specified by adding it to the options for cmake via the
`DCMAKE_CXX_FLAGS` option.
however as far as i can tell it doesn't set it itself.
i'm wondering if we just need to remove this flag altogether and not set any standard.
there's even a comment that c++14 doesn't work for some configurations.
That was 6 years ago. I am pretty sure it is outdated.
Does google test specify the minimal required C++ standard in it's CMake Interface?
No, otherwise those almighty CI bots would have failed.
however as far as i can tell it doesn't set it itself.
Well, they should. Someone should fix that first then.
Standard version required to build the library, and the standard required to use the library are not necessarily the same thing, googletest just needs to advertise/specify which standard version is needed when linking to it.
if we set CMAKE_CXX_STANDARD
I didn't say CMAKE_CXX_STANDARD, I said CXX_STANDARD. See: https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html
yeah you did... but googletest suggests setting CMAKE_CXX_STANDARD in their docs and given we're trying to align with them on your suggestion...
and the font of all knowledge stack overflow agrees: https://stackoverflow.com/questions/10851247/how-do-i-activate-c-11-in-cmake
Well, they should. Someone should fix that first then.
It will break those infamous CI bots for Google Benchmark. No, Google Benchmark should be fixed first.
stack overflow agrees
This is a 7-years old post. Modern CMake's approach is all about targets.
#1464 has the results of some tests. unsurprisingly, once again, we're not able to take advantage of modern CMake functionality because we need to support older platforms.
i'm open to ideas but i'm a bit stumped.
I can only repeat what i have already said: gtest needs to set the C++ standard needed to build it, and advertise (in it's target interface properties) the required C++ standard to use it. Does it do that?
I can only repeat what i have already said: gtest needs to set the C++ standard needed to build it, and advertise (in it's target interface properties) the required C++ standard to use it. Does it do that?
Why do you have to repeat it? I agree, and I've already said that it doesn't.
So once it does, let's cycle back to the google benchmark side of the problem.
we're not able to take advantage of modern CMake functionality because we need to support older platforms
You should check your answers against simple, verifiable facts. Google Benchmark's CMakeLists.txt requires CMake 3.5. CXX_STANDARD is available in CMake 3.5. See: https://cmake.org/cmake/help/v3.5/prop_tgt/CXX_STANDARD.html
So once it does, let's cycle back to the google benchmark side of the problem.
This is a strategy that will not help anybody. Changing GTest first will break Google Benchmark builds. It will be much easier for everyone to change Google Benchmark first. Note, I am a third party here, I don't actually care, it's just a suggestion.
i'm open to ideas but i'm a bit stumped.
Remove setting the standard altogether. This is usually used for programs that use new features, not vice versa, requesting everyone to use the old one.
@intractabilis can you please take your attitude down, like, 4 notches. you've come in with a bunch of bluster to an open source project that is staffed by volunteers and thrown your opinions around like you own the place. step back for a bit, calm down, and then let's discuss options if you want to. if you want to just throw the problem over the wall and walk off, that's cool too. thank you for your report, we'll take it from here.
in terms of facts: https://github.com/google/benchmark/pull/1464: yes, CXX_STANDARD is available in 3.5 but it doesn't get passed through to try_compile, which means it's basically useless. if we can't test the platform we're on, we can't build for it safely. that safety only comes in 3.8+ and we're currently discussing on the PR whether we can bump our cmake dependency requirement.
unsurprisingly, we have a lot of different toolchains and platforms to consider as users so these changes are not trivial to consider.
I apologize for being a bad communicator.
Why don't you want to just remove add_cxx_compiler_flag(-std=c++11) ? I am not sure what problem the author of that line was trying to solve. MSVC compiler, obviously, lived without it (or anything similar) for years. The purpose of CXX_STANDARD is to make a program that uses new C++ features to fail to compile with a graceful message on a toolset that doesn't support new features. I.e. in the case of this project that would be a situation, if somebody would try to compile Google Benchmark with at least a 12-year-old compiler. I guess, if somebody does it, they wouldn't need a graceful message warning them a 12-year-old compiler probably doesn't support modern C++, it would be obvious. Therefore, why? We should rely on reasonable defaults given by the toolsets and CMake, attempts to oversmart the reasonable defaults always lead to maintenance problems like this one.
no problem. i think there's been some difficult communication all around. moving on :)
my first patch on the linked PR did exactly that (just removed the flag) but that ends up with compilers falling back to older standards by default. perhaps we don't care about platforms that do that, but given we have a tool in cmake (CXX_STANDARD) to document the expected minimum standard required to build the project, it seems reasonable to attempt to use it.
i agree we should move from the flag to CXX_STANDARD. the only question is how we do that given the (currently probably too broad) support for older platforms. it may be we just have to shrug and say "this is too old now".
note that we don't need C++11 to run the library, only to build and run tests. we've been careful to keep c++11 out of the public API (i think... maybe something's slipped in by mistake) so it's possible the older platform support is less important.
my first patch on the linked PR did exactly that (just removed the flag) but that ends up with compilers falling back to older standards by default
To C++03?
As i have said, in general, any code that you compile, isn't infinitely back- and forward- compatible, it is compatible with //some// C++ standard version, so unless you like breakage, you need to specificly ask for that specific version when compiling. The C++ standard version to which the compilers happen to fall back to is irrelevant, it is not guaranteed that they are sane, or match your expectations.
so unless you like breakage, you need to specificly ask for that specific version when compiling
If you want new features, yes. You shouldn't downgrade the compiler. C++ compilers are backward compatible. There are maybe incompatibilities between different vendors due to bugs or misinterpretation of the standard, but they should be resolved by other means than enforcing everyone to use only C++11. Google Benchmark + GTest compile just fine with C++20.
@dominichamon If some compilers we support fall back to C++03, we should check in CMake for that compilers specifically and set C++14 for them. If they don't support C++14, we cannot support them, obviously, because GTest requires now C++14.
isn't that exactly the point of the CXX_STANDARD flag in cmake? "we would like to set the standard to X". and CXX_STANDARD_REQUIRED says "don't fall back further than this"?
or did i misunderstand the point of these settings?
i think this will be resolved once i can land the cmake version bump (#1468 .. currently seeing an ABI mismatch in the cxx03_test) and then can set the relevant cmake CXX_STANDARD flags to require the minimum we expect (C++11 for now) for building the project.
@intractabilis in #1468 i'm moving the standard setting to the targets, as you have suggested above. my only concern with this is that it doesn't then propagate the standard setting to the try_compile and try_run feature checks as they're not linked to the targets...
and in fact it doesn't work as i'd hoped (i was hoping to have the tests run against a cxx03 ABI to confirm that we don't need c++11 to run them).
CXX_STANDARD is just a compiler-independent way to specify -std=c++XX, no more, no less. I'll comment on the rest later.
and in fact it doesn't work as i'd hoped (i was hoping to have the tests run against a cxx03 ABI to confirm that we don't need c++11 to run them).
Can you point me at the problem?
and in fact it doesn't work as i'd hoped (i was hoping to have the tests run against a cxx03 ABI to confirm that we don't need c++11 to run them).
Can you point me at the problem?
https://github.com/google/benchmark/runs/7793041669?check_suite_focus=true
and in fact it doesn't work as i'd hoped (i was hoping to have the tests run against a cxx03 ABI to confirm that we don't need c++11 to run them).
Can you point me at the problem?
https://github.com/google/benchmark/runs/7793041669?check_suite_focus=true
I guess that is #672?
yes, or closely related.
i'm trying to decide if it's useful to resolve this to have the source built under c++1X and the tests built and run under cxx03 or if we're beyond needing to support this...