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

Use CMAKE_CXX_STANDARD when available #953

Merged
merged 2 commits into from Nov 16, 2020
Merged

Use CMAKE_CXX_STANDARD when available #953

merged 2 commits into from Nov 16, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 15, 2020

If the use has provided the variable CMAKE_CXX_STANDARD use that instead of providing a default cache variable.

If the use has provided the variable CMAKE_CXX_STANDARD use that instead of providing a default cache variable.
@ghost
Copy link

ghost commented Nov 15, 2020

CLA assistant check
All CLA requirements met.

@ghost
Copy link
Author

ghost commented Nov 15, 2020

This change does a couple of things.

It creates a cmake module to assist in writing helper functions/macros.

Creates 2 new function:

  • gsl_set_default_cxx_standard
  • gsl_client_set_cxx_standard

gsl_set_default_cxx_standard does exactly what the old code did before.

gsl_client_set_cxx_standard checks if the client set the CMAKE_CXX_STANDARD.

If the client has set CMAKE_CXX_STANDARD then we should avoid doing anything else.

  1. It's easier for clients.
  2. It's less error prone.
  3. Avoid extra configuration time.
  4. Avoid unnecessary cache variable overriding.

@ghost
Copy link
Author

ghost commented Nov 15, 2020

Seems like the IOS builds have been failing for a while. And aren't related to this PR.

@JordanMaples
Copy link
Contributor

Yeah, the iOS failures are unrelated. I'm not sure why the Appveyor run exploded like that.
I'll test the changes locally before merging.

@Xazax-hun
Copy link

These changes look good to me. In general, I prefer refactoring changes and functional changes separated in different commits, but this is just a nit :)

@JordanMaples
Copy link
Contributor

JordanMaples commented Nov 16, 2020

I looked into the Appveyor failure since it's happening across all builds. There's no issue with this PR and as far as I can tell your changes work, so I'm going to merge it. (I am by no means a CMake expert though)

In case anyone is interested in the Appveyor failure.
The issue is that the compiler version that Appveyor is using was updated from Visual Studio v16.7.6 to v16.8.1 which defaults from Clang 10.0.0 to Clang 11.0.0. GoogleTest is emitting a -Wsuggest-override warning in Clang 11.

@JordanMaples JordanMaples merged commit 475b785 into microsoft:master Nov 16, 2020
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.

2 participants