Upgrade to C++17 #3103
Upgrade to C++17 #3103
Conversation
|
The very minimum we need is to make sure all of our CI and builders support enough c++17 for this (which includes the chromium CI and CI here). Secondarily, we know some users build from source and we'd like to not break them. Like most open source projects we don't have a perfect idea of who those users are. But maybe that should include that recent versions of Ubuntu and MSVC work out of the box. For Ubuntu, I assume the new LTS (20.04) is ok, but it would be good to know if the previous one (18.04) also is (the one before, 16.04, definitely isn't, but that doesn't worry me as it already doesn't work even before c++17). I've thought that a wasm2c build could help here (as that would definitely build everywhere) but have not had time to set up CI for that. |
|
It looks like the default compiler on Ubuntu 18.4 is gcc 7.4, which supports almost all of C++17. It also looks like basically all of C++17 is supported on MSVC since VS 2017 15.7, although some defect reports are only supported since VS 2019 (or not supported at all). So compiler distribution coverage seems reasonable to me. I'm not sure of an easy way to check compatibility on the Chromium CI besides landing this and seeing if anything breaks. |
|
Sounds good. For Chromium CI, I think we are ok, the Binaryen build log says, |
|
Leaving the merge button to you guys :) |
|
🤞 |
| @@ -188,7 +192,7 @@ else() | |||
| set(THREADS_PREFER_PTHREAD_FLAG ON) | |||
| set(CMAKE_THREAD_PREFER_PTHREAD ON) | |||
| find_package(Threads REQUIRED) | |||
| add_cxx_flag("-std=c++14") | |||
| add_cxx_flag("-std=c++${CXX_STANDARD}") | |||
There was a problem hiding this comment.
Do you know why we need this in addition to the CXX_STANDARD property?
|
Looks like this broke Chromium CI so we should probably revert it for now. |
|
I believe this is because we build using a debian/stretch sysroot on those builders. The reason we use a sysroot is because goma requires it. We could either stop using goma for binaryen or build a new sysroot. |
|
(BTW its a C++17 library issue with libstdc++ in the sysroot, the compiler itself has full C++17 support). |
Since this came up again lately in context of using
std::variant, I took a look at comments in #2709 and thought why not give it a try and see what CI is going to say. Refactors the standard level into variables so it's trivial to upgrade in the future.