Minimum c++ std to c++17 #3348

PDoakORNL · 2021-08-06T21:05:20Z

Proposed changes

Make the default c++ standard 17.
Close #2790
Closes #2699

Still many build recipes to update in the docs, feel free to pitch in on your favorite.

What type(s) of changes does this code introduce?

Delete the items that do not apply

New feature
Build related changes
Documentation changes

Does this introduce a breaking change?

No, unless your compiler is old

What systems has this change been tested on?

Checklist

Yes. This PR is up to date with current the current state of 'develop'
Yes. Code added or changed in the PR has been clang-formatted
Yes. This PR adds tests to cover any new code, or to catch a bug that is being fixed
Yes. Documentation has been added (if appropriate)

ye-luo

The PR was not made clean.

ye-luo · 2021-08-06T21:12:06Z

CMakeLists.txt

@@ -410,7 +410,7 @@ include(CMake/Testlibstdc++.cmake)
 if(QMC_CXX_STANDARD GREATER_EQUAL 17)
  include(CMake/TestCxx17Library.cmake)
 else()
-  include(CMake/TestCxx14Library.cmake)
+  message(FATAL_ERROR "QMCPACK requires compiler support of c++17 or greater.")


This function is not responsible for requesting C++17. No change here. If C++14 is needed, it should be allowed. That is why the top level only issues a warning.

The point is to stop writing workaround code in c++14 and use the simpler semantics of c++17. So it should be a fatal error to request less than 17 for the standard.

I think we still want to give a warning for higher standard numbers like we have in the past.

Lets make this even easier. Change the top-level CMakeLists.txt warning message to an error. We can then remove the 14 check in Testliststdc++.cmake

We don't live in a perfect world... The warning is clear, it is on your own. But we should not add even more hurdles when sb is struggling with C++14 need.

NVHPC since 20.7 has supported c++17
I think that was the first release

But I don't know how to really support NVHPC without cmake CUDA as a language

NVHPC should work without CUDA. Imagine you hit a bug, it may caused by the CPU part.
NVHPC right now should work with the current CUDA setting.

We drive NVHPC just like gcc. Nothing to worry about for this PR.

So is this resolved? Now we just check for a good c++17 library.

prckent · 2021-08-06T21:24:48Z

README.md needs an update

williamfgc · 2021-08-06T21:39:38Z

@PDoakORNL FYI, I canceled the workflows from previous commits in GitHub Actions as this PR evolved quickly, I just kept the head commit to avoid competing for resources, see here.

prckent · 2021-08-09T19:46:29Z

How about I try this with the nightly build combinations, and if they work we consider this good to merge?

prckent · 2021-08-10T16:34:30Z

I tried the nightly build configurations for sulfur. Everything worked except for clangdev (clang version 13.0.0 (https://github.com/llvm/llvm-project 13ccb097258a244498aa760b878a23de721af29f)
) which turns out to be fresh compiler bug compiling WriteEshdf.cpp .

git_QMCPACK_prckent]$ grep -e "START build configuration" -e "% tests passed" out | sed -e 's/.*configuration//g' -e 's/ Tue.*//g'
 build_gccnew
100% tests passed, 0 tests failed out of 832
 build_intel2020_nompi
100% tests passed, 0 tests failed out of 652
 build_intel2020
100% tests passed, 0 tests failed out of 832
 build_intel2020_complex
100% tests passed, 0 tests failed out of 767
 build_intel2020_mixed
99% tests passed, 1 tests failed out of 749
 build_intel2020_complex_mixed
99% tests passed, 1 tests failed out of 729
 build_gccnew_nompi_mkl
100% tests passed, 0 tests failed out of 652
 build_gccold_nompi_mkl
100% tests passed, 0 tests failed out of 630
 build_clangnew_nompi_mkl
100% tests passed, 0 tests failed out of 652
 build_gccnew_nompi
100% tests passed, 0 tests failed out of 652
 build_clangnew_nompi
100% tests passed, 0 tests failed out of 652
 build_gccnew_mkl
100% tests passed, 0 tests failed out of 832
 build_gccnew_mkl_complex
100% tests passed, 0 tests failed out of 767
 build_gccdev_mkl
100% tests passed, 0 tests failed out of 832
 build_gccdev_mkl_complex
100% tests passed, 0 tests failed out of 767
 build_clangnew_mkl
100% tests passed, 0 tests failed out of 832
 build_clangnew_mkl_complex
100% tests passed, 0 tests failed out of 767
 build_clangnew_mkl_mixed
100% tests passed, 0 tests failed out of 749
 build_gcclegacycuda
100% tests passed, 0 tests failed out of 240
 build_gcclegacycuda_complex
100% tests passed, 0 tests failed out of 254
 build_gcclegacycuda_full
100% tests passed, 0 tests failed out of 240
 build_pgi2021_nompi
99% tests passed, 3 tests failed out of 652
 build_gccnew_debug_mkl
100% tests passed, 0 tests failed out of 832
 build_gccnew_debug_complex_mkl
100% tests passed, 0 tests failed out of 767
 build_clangdev_nompi_mkl
0% tests passed, 652 tests failed out of 652
 build_clangdev_nompi_mkl_complex
0% tests passed, 573 tests failed out of 573
 build_clangdev_offloadcuda_nompi_mkl
0% tests passed, 652 tests failed out of 652
 build_clangdev_offloadcuda_nompi_mkl_complex
0% tests passed, 573 tests failed out of 573

prckent · 2021-08-10T16:35:34Z

I'll try with a fresh llvm but I do wonder if anyone else has seen this. Since the problem is with the development compiler we could just ignore it.

prckent · 2021-08-10T16:36:45Z

In the above, Intel is 19.1.3.304

ye-luo · 2021-08-10T17:10:09Z

I'll try with a fresh llvm but I do wonder if anyone else has seen this. Since the problem is with the development compiler we could just ignore it.

that commit has been broken. I believe the previous bad commit has been reverted somewhere after this commit.

prckent · 2021-08-10T17:36:06Z

Fresh llvm@main install failed via spack due to llvm-omp-device-info tool failure.

I think we can call QMCPACK capable for C++17 with this status.

prckent · 2021-08-10T20:41:41Z

Spotted a "(I think its actually higher)" that needs to be removed.

ye-luo · 2021-08-10T21:01:18Z

Spotted a "(I think its actually higher)" that needs to be removed.

Fixed.

ye-luo · 2021-08-10T21:01:42Z

Start testing in-house

prckent · 2021-08-11T01:18:05Z

Start testing in-house

ye-luo · 2021-08-11T02:19:53Z

Test this please

williamfgc · 2021-08-11T11:51:02Z

@PDoakORNL I tried triggering GitHub Action hosted runners, but the results don't propagate to these PR checks dashboard which look messed up. They might have to be launched with a new commit push: git commit --amend and git push -f <your-remote> drop_c++14.

ye-luo · 2021-08-11T16:00:33Z

Start testing in-house

PDoakORNL changed the title ~~Minmum c++ std to c++17~~ Minimum c++ std to c++17 Aug 6, 2021

ye-luo requested changes Aug 6, 2021

View reviewed changes

PDoakORNL added 2 commits August 6, 2021 17:14

the easy part

ad060ce

partial update of documenation for c++17

39ef7ac

PDoakORNL force-pushed the drop_c++14 branch from 1b0a4b7 to 39ef7ac Compare August 6, 2021 21:14

update libstdc++ compatibility check

9046c86

PDoakORNL added 2 commits August 6, 2021 17:25

simplified check

0a12b19

update readme

70f2cf5

Update Changlog

2713e50

prckent self-requested a review August 10, 2021 17:39

prckent previously approved these changes Aug 10, 2021

View reviewed changes

ye-luo added 2 commits August 10, 2021 15:27

More flexible check and remove used file.

173919b

Merge remote-tracking branch 'origin/develop' into drop_c++14

5fad328

ye-luo dismissed prckent’s stale review via 5fad328 August 10, 2021 20:27

Correct docs.

d6715b1

ye-luo approved these changes Aug 10, 2021

View reviewed changes

ye-luo enabled auto-merge August 10, 2021 23:07

Merge branch 'develop' into drop_c++14

2000a0c

ye-luo merged commit 5716f3e into QMCPACK:develop Aug 11, 2021

PDoakORNL deleted the drop_c++14 branch February 23, 2022 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimum c++ std to c++17 #3348

Minimum c++ std to c++17 #3348

PDoakORNL commented Aug 6, 2021 •

edited by ye-luo

Loading

ye-luo left a comment

ye-luo Aug 6, 2021

PDoakORNL Aug 6, 2021

PDoakORNL Aug 6, 2021

prckent Aug 6, 2021

ye-luo Aug 6, 2021 •

edited

Loading

PDoakORNL Aug 6, 2021

PDoakORNL Aug 6, 2021

ye-luo Aug 6, 2021

prckent Aug 6, 2021

PDoakORNL Aug 9, 2021

prckent commented Aug 6, 2021

williamfgc commented Aug 6, 2021

prckent commented Aug 9, 2021

prckent commented Aug 10, 2021

prckent commented Aug 10, 2021

prckent commented Aug 10, 2021

ye-luo commented Aug 10, 2021

prckent commented Aug 10, 2021 •

edited

Loading

prckent commented Aug 10, 2021

ye-luo commented Aug 10, 2021

ye-luo commented Aug 10, 2021

prckent commented Aug 11, 2021

ye-luo commented Aug 11, 2021

williamfgc commented Aug 11, 2021

ye-luo commented Aug 11, 2021

Minimum c++ std to c++17 #3348

Minimum c++ std to c++17 #3348

Conversation

PDoakORNL commented Aug 6, 2021 • edited by ye-luo Loading

Proposed changes

What type(s) of changes does this code introduce?

Does this introduce a breaking change?

What systems has this change been tested on?

Checklist

ye-luo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ye-luo Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prckent commented Aug 6, 2021

williamfgc commented Aug 6, 2021

prckent commented Aug 9, 2021

prckent commented Aug 10, 2021

prckent commented Aug 10, 2021

prckent commented Aug 10, 2021

ye-luo commented Aug 10, 2021

prckent commented Aug 10, 2021 • edited Loading

prckent commented Aug 10, 2021

ye-luo commented Aug 10, 2021

ye-luo commented Aug 10, 2021

prckent commented Aug 11, 2021

ye-luo commented Aug 11, 2021

williamfgc commented Aug 11, 2021

ye-luo commented Aug 11, 2021

PDoakORNL commented Aug 6, 2021 •

edited by ye-luo

Loading

ye-luo Aug 6, 2021 •

edited

Loading

prckent commented Aug 10, 2021 •

edited

Loading