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

GitHub-CI: test CPP17 #13508

Merged
merged 1 commit into from Mar 10, 2022
Merged

GitHub-CI: test CPP17 #13508

merged 1 commit into from Mar 10, 2022

Conversation

peterrum
Copy link
Member

@peterrum peterrum commented Mar 7, 2022

As discussed with @tjhei.

Tempted by #13497 and #13502.

@peterrum peterrum added the Tests label Mar 7, 2022
Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

Good idea!

@@ -68,7 +68,7 @@ jobs:
mkdir build
cd build
cmake -D CMAKE_BUILD_TYPE=Debug \
-D DEAL_II_CXX_FLAGS='-Werror' \
-D DEAL_II_CXX_FLAGS='-Werror -std=c++17' \
Copy link
Member

Choose a reason for hiding this comment

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

We could also use the CMAKE functionality. Is there a difference? Might be just portability.
I'm actually undecided what I prefer personally :-)

Suggested change
-D DEAL_II_CXX_FLAGS='-Werror -std=c++17' \
-D CMAKE_CXX_STANDARD=17 \
-D DEAL_II_CXX_FLAGS='-Werror' \

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed this!

Copy link
Member

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

This just switches one of the testers to C++17. Ideally, or course, we would add a separate CI step that adds a C++17 tester, but that increases the wait time we have (or exceeds whatever limit we are allotted). How did you choose this particular tester to switch to C++17? I supposed this leaves enough testers who continue to check C++14.

@peterrum
Copy link
Member Author

peterrum commented Mar 8, 2022

This just switches one of the testers to C++17. Ideally, or course, we would add a separate CI step that adds a C++17 tester, but that increases the wait time we have (or exceeds whatever limit we are allotted). How did you choose this particular tester to switch to C++17? I supposed this leaves enough testers who continue to check C++14.

This is how we normally do it. If we see that something is not tested (in the past: python bindings, 64-bit indices), we just modify one test. We still have enough tests with C++14. Adding a new test would be an option, but would mean longer queue times.

@masterleinad
Copy link
Member

I don't think setting CMAKE_CXX_STANDARD works. I still see

#        C++ language standard:  C++14

in the respective CI run. We should rather do DEAL_II_CXX_FLAGS=-std=c++17.

@marcfehling
Copy link
Member

marcfehling commented Mar 8, 2022

I don't think setting CMAKE_CXX_STANDARD works. I still see

#        C++ language standard:  C++14

in the respective CI run. We should rather do DEAL_II_CXX_FLAGS=-std=c++17.

Interesting. I think in addition to CMAKE_CXX_STANDARD we would need to set CXX_STANDARD_REQUIRED=ON, see also here. Or we can specify the flags for the gnu compiler directly.

@masterleinad
Copy link
Member

I think in addition to CMAKE_CXX_STANDARD we would need to set CXX_STANDARD_REQUIRED=ON, see also here. Or we can specify the flags for the gnu compiler directly.

That isn't enough for me. I think I remember that #10307 ignores the CMAKE_CXX_STANDARD completely and only relies on flags we pass and the default behavior of the compiler to decide what kind of language version support we have.

@peterrum
Copy link
Member Author

CI says now: C++ language standard: C++17!

@masterleinad masterleinad merged commit 6f2ba8f into dealii:master Mar 10, 2022
@bangerth
Copy link
Member

Nice! Now we just need a tester for C++20 ;-)

@peterrum
Copy link
Member Author

Nice! Now we just need a tester for C++20 ;-)

@bangerth Couldn't we just simply convert the C++17 to a C++20 test? What would we miss?

@bangerth
Copy link
Member

We probably could, but I don't think we use any C++20 features yet (even with the appropriate #ifdef). So I think there is, at least for now, not a lot of use for this.

But that might change in the future, of course. Do we have a tester that already has a compiler that's new enough? I think we'd need GCC 11 at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants