Skip to content
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

Try removing attempt to set the C++ standard #1464

Merged
merged 11 commits into from
Dec 10, 2022
Merged

Try removing attempt to set the C++ standard #1464

merged 11 commits into from
Dec 10, 2022

Conversation

dmah42
Copy link
Member

@dmah42 dmah42 commented Aug 8, 2022

Fixes #1460 #1462

Let's see what the buildbots make of this.

@LebedevRI
Copy link
Collaborator

No, this is wrong.

@dmah42
Copy link
Member Author

dmah42 commented Aug 8, 2022

No, this is wrong.

it's just a draft because i want to see what happens... the compiler used should be free to set it's standard. we already had a fallback to c++0x so the only issue i think would be if someone tries to use a compiler that doesn't support c++11.

if we want to require that then setting CMAKE_CXX_STANDARD and maybe REQUIRED is the way to do that.. but i just want to see how necessary it is.

@LebedevRI
Copy link
Collaborator

There is absolutely no requirement that a supported compiler version actually happens
to have the right (the one you expect, and everyone expects different things) default standard.

@dmah42
Copy link
Member Author

dmah42 commented Aug 8, 2022

first test complete: OSX falls back too far beyond C++11.

next up: setting the standard.

@LebedevRI
Copy link
Collaborator

@dmah42
Copy link
Member Author

dmah42 commented Aug 8, 2022

https://cmake.org/cmake/help/latest/policy/CMP0067.html might be relevant.

nice :)

@@ -137,6 +138,9 @@ if (BENCHMARK_BUILD_32_BITS)
add_required_cxx_compiler_flag(-m32)
endif()

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably also want CMAKE_CXX_EXTENSIONS OFF

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah.. i'm also eyeing up the check where we:

set(EXTRA_CXX_FLAGS "")
if (WIN32 AND "${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
  # Clang on Windows fails to compile the regex feature check under C++11
  set(EXTRA_CXX_FLAGS "-DCMAKE_CXX_STANDARD=14")
endif()

and wondering how that will end up interacting with this change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes FWIW we need this check for clang on windows.

@dmah42
Copy link
Member Author

dmah42 commented Aug 8, 2022

https://cmake.org/cmake/help/latest/policy/CMP0067.html might be relevant.

nice :)

cmake 3.8+ only .. ubuntu 16.04 is still on 3.5.1: https://launchpad.net/ubuntu/xenial/+source/cmake

@dmah42
Copy link
Member Author

dmah42 commented Aug 9, 2022

try_compile also doesn't support setting the standard in 3.5.1.

so our older platform support requires explicit --std flag setting, i think... and for consistency, it suggests we need to do the same in the rest of the project.

until we can drop older platforms, i don't think we can embrace updated cmake norms...

any other ideas, @LebedevRI ?

@LebedevRI
Copy link
Collaborator

cmake 3.5.1 sounds really ancient.
Even debian old-old-stable has 3.7.
I think 3.13 should be an unambiguous dependency.

@dmah42
Copy link
Member Author

dmah42 commented Aug 9, 2022

ubuntu bionic (the oldest LTS ubuntu we support) is 3.10.2 (https://packages.ubuntu.com/bionic/cmake) so i think we can get past 3.8.

it might mean dropping the 14.04 and 16.04 bots but they are really old at this point and we do have a policy on dependencies for a reason.

@LebedevRI
Copy link
Collaborator

Sounds reasonable?

@@ -137,6 +139,10 @@ if (BENCHMARK_BUILD_32_BITS)
add_required_cxx_compiler_flag(-m32)
endif()

set(CMAKE_CXX_STANDARD 11)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not meaningful. MSVC, for instance, does not provide exclusive C++11 support, only C++14 and later: https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=msvc-170. CMake will probably decay to the newer standard anyway.

@dmah42
Copy link
Member Author

dmah42 commented Aug 10, 2022

Sounds reasonable?

image

seems like we might just have had our hand forced...

@LebedevRI
Copy link
Collaborator

@dmah42 what's the status here?

@dmah42
Copy link
Member Author

dmah42 commented Dec 10, 2022

@dmah42 what's the status here?

i don't remember where i got to. let me see if i can resurrect it.

@dmah42
Copy link
Member Author

dmah42 commented Dec 10, 2022

it builds and passes tests but I'm not sure it's the right thing to do. I would appreciate an ack.

@LebedevRI
Copy link
Collaborator

it builds and passes tests but I'm not sure it's the right thing to do. I would appreciate an ack.

Given that we don't want to just happen to build under whatever the standard is default
for the compiler that is used, but specifically with C++11, this is the right thing to do.

Copy link
Collaborator

@LebedevRI LebedevRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@dmah42 dmah42 marked this pull request as ready for review December 10, 2022 23:42
@dmah42 dmah42 merged commit da652a7 into main Dec 10, 2022
@dmah42 dmah42 deleted the std branch December 10, 2022 23:42
@dmah42
Copy link
Member Author

dmah42 commented Dec 10, 2022

merged.. now let's see if anyone finds an issue on any unusual platforms with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] requirement of C++11 is incompatible with Google Test
5 participants