Skip to content

use C++17#703

Merged
cnpetra merged 2 commits intodevelopfrom
support-cpp17-dev
Feb 10, 2025
Merged

use C++17#703
cnpetra merged 2 commits intodevelopfrom
support-cpp17-dev

Conversation

@nychiang
Copy link
Collaborator

@nychiang nychiang commented Jan 24, 2025

Set C++17 as the standard in CMake files

@nychiang nychiang marked this pull request as ready for review January 31, 2025 17:10
@nychiang nychiang requested a review from cnpetra January 31, 2025 17:11
@cnpetra
Copy link
Collaborator

cnpetra commented Feb 6, 2025

@nychiang can you please confirm CMAKE_CXX_STANDARD is configurable from command line and spack?

CMakeLists.txt Outdated
Comment on lines +5 to +6
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++17 -lstdc++fs ")

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if this is portable. It will probably not work with MSVC, for example.

Kitware suggests adding language standards to targets such as

target_compile_features(HIOP PRIVATE cxx_std_17)

My understanding is that the built-in target cxx_std_17 is supposed to add correct flags for the compiler of your selection.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pelesh We are worrying about if this PR will affect ExaGO, as it uses c++14. see here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt it is going to be an issue, because ExaGO only links to HiOp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

after checking others' code, e.g., RAJA, MFEM, etc..., I don't think I need to specify -std=c++17 or target_compile_features. Just pushed another commit for review.

@cnpetra cnpetra merged commit 36d9814 into develop Feb 10, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants