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.

Sign up for GitHub

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

Jump to bottom

build: move from c++11 to c++17 #4983

Merged
merged 2 commits into from
Jan 20, 2023
Merged

build: move from c++11 to c++17 #4983

merged 2 commits into from
Jan 20, 2023

Conversation

Keenuts
Copy link
Contributor

@Keenuts Keenuts commented Nov 7, 2022

Moving from cpp 11 to cpp 17 will allow us to use modern features like filesystem support, optional, any, execution policies and improved templates.

@Keenuts Keenuts marked this pull request as ready for review November 7, 2022 17:11
@Keenuts
Copy link
Contributor Author

Keenuts commented Nov 7, 2022

Main question to answer: will we break something that cannot be fixed? (DXC is moving forward with cpp17 by ex)

dneto0
dneto0 reviewed Dec 16, 2022
View reviewed changes
Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

See also line 34, and #5020

Is this specific to clang and GCC? I suppose we have to leave the C++11 flow for MSVC?

@Keenuts
Copy link
Contributor Author

Keenuts commented Jan 3, 2023 •
edited
Loading

See also line 34, and #5020
Is this specific to clang and GCC? I suppose we have to leave the C++11 flow for MSVC?

The idea was to align this repo to LLVM (and now DXC) 3 years "rule": allow compiler/toolchains/standard bump after ~3 years.
Thanks for pointing out #5020. Updated this PR to work with this change.

Vulkan-validation-layers already builds with cpp17, shaderc can build in cpp17, Dawn still builds when changing spirv-tools to cpp17, and DXC dropped support for pre-2022 VS, so won't break either.

@Keenuts Keenuts force-pushed the cpp branch 2 times, most recently from eb997d7 to d7c478d Compare January 5, 2023 16:09
vettoreldaniele
vettoreldaniele reviewed Jan 16, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@s-perron
Copy link
Collaborator

We have agreed to move forward with this. We just want to hear if there are any objections from the spir working group.

We believe this should be acceptable because:

  1. The vulkan validation layers have already moved to c++17, so the eco system is moving that way.
  2. The eco system survey shows that 95% of Vulkan developers use VS2019 or later, so their compilers support C++17.

There are a few other changes that will be needed before we can merge this.

Some kokoro file checkout a specific version of googletest instead of the latest for the kokoro builds. I think we should be able to remove that:

./kokoro/macos-clang-release-bazel/build.sh
./kokoro/check-format/build.sh
./kokoro/windows-msvc-2017-release-bazel/build.bat

We will also have to update the README to say that we use C++17 (https://github.com/KhronosGroup/SPIRV-Tools#usage).

@dneto0
Copy link
Collaborator

dneto0 commented Jan 18, 2023

We have agreed to move forward with this. We just want to hear if there are any objections from the spir working group.

In the 2022-01-18 SPIR WG meeting, the group agreed to move to C++17 minimum.
Thanks!

@Keenuts
Copy link
Contributor Author

Keenuts commented Jan 18, 2023

Thanks! Updated the README to say we use c++17, and support VS2019 (added a note saying we won't drop support for VS2017 until it breaks due to incomplete support).

This was referenced Jan 18, 2023
Merged
Merged
@Keenuts Keenuts force-pushed the cpp branch 2 times, most recently from cc1095b to 2c1885d Compare January 18, 2023 18:21
s-perron
s-perron reviewed Jan 18, 2023
View reviewed changes
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

As a follow up or part of this change, we should start building with the latest googletest again. This would mean updating the DEPS file and the roll_deps.sh.

s-perron
s-perron reviewed Jan 18, 2023
View reviewed changes
README.md Show resolved Hide resolved
s-perron
s-perron reviewed Jan 18, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@Keenuts
build: move from c++11 to c++17
09b62c0 
Moving from cpp 11 to cpp 17 will allow us to use modern features like
filesystem support, optional, any, execution policies and improved
templates.

Signed-off-by: Nathan Gauër <brioche@google.com>
dneto0
dneto0 requested changes Jan 19, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
dneto0
dneto0 approved these changes Jan 19, 2023
View reviewed changes
Copy link
Collaborator

@dneto0 dneto0 left a comment

Choose a reason for hiding this comment

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

Thanks

@Keenuts Keenuts merged commit cdc4e52 into KhronosGroup:main Jan 20, 2023
@Keenuts Keenuts deleted the cpp branch January 20, 2023 14:18
SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request Apr 10, 2023
@SpaceIm
spirv-tools requires C++17 since 1.3.243.0
56c69bc 
see KhronosGroup/SPIRV-Tools#4983
conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Apr 13, 2023
@SpaceIm
(#16959) spirv-tools: add 1.3.243.0 + remove old versions
ab6c62e 
* add spirv-tools/1.3.243.0

* remove maintenance of old versions

* spirv-tools requires C++17 since 1.3.243.0

see KhronosGroup/SPIRV-Tools#4983

* raise if compiler version not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s-perron s-perron s-perron left review comments

@vettoreldaniele vettoreldaniele vettoreldaniele left review comments

@dneto0 dneto0 dneto0 approved these changes

@alan-baker alan-baker Awaiting requested review from alan-baker

@dnovillo dnovillo Awaiting requested review from dnovillo

@greg-lunarg greg-lunarg Awaiting requested review from greg-lunarg

@cassiebeckley cassiebeckley Awaiting requested review from cassiebeckley

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Keenuts @s-perron @dneto0 @vettoreldaniele