Skip to content

Use CMAKE_CXX_STANDARD when available#953

Merged
JordanMaples merged 2 commits into
masterfrom
unknown repository
Nov 16, 2020
Merged

Use CMAKE_CXX_STANDARD when available#953
JordanMaples merged 2 commits into
masterfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Nov 15, 2020

Copy link
Copy Markdown

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

ghost commented Nov 15, 2020

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

@ghost

ghost commented Nov 15, 2020

Copy link
Copy Markdown
Author

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

ghost commented Nov 15, 2020

Copy link
Copy Markdown
Author

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

@JordanMaples

Copy link
Copy Markdown
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
Copy Markdown

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

JordanMaples commented Nov 16, 2020

Copy link
Copy Markdown
Contributor

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