-
Notifications
You must be signed in to change notification settings - Fork 362
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
Fix CMake Compiler Flag Summary: ID Regex Matching #4272
base: development
Are you sure you want to change the base?
Conversation
@@ -279,7 +279,7 @@ function ( eval_genex _list _lang _comp ) | |||
string(REGEX REPLACE "\\$<PLATFORM_ID:[A-Za-z]*>" "0" _in "${_in}") | |||
|
|||
# Genex in the form $<*_COMPILER_ID:compiler_ids> | |||
string(REGEX REPLACE "\\$<${_lang}_COMPILER_ID:[^>]*${_comp}[^>]*>" "1" _in "${_in}") | |||
string(REGEX REPLACE "\\$<${_lang}_COMPILER_ID[^>]*[:,]${_comp}[>,]" "1" _in "${_in}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand what the change in regex does for each part?
- Does
[:,]
now match a literal:
or,
? - Does
[>,]
now match a literal>
or,
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Does
[:,]
now match a literal:
or,
?- Does
[>,]
now match a literal>
or,
?
Yes to both, the idea is that we will now exactly (without prefixes or suffixes) match ${_comp}
because it must be surrounded by commas (if it's in the middle of the list) or immediately follow the colon at the beginning of the list or immediately precede the chevron at the end of the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also match $<CXX_COMPILER_ID,someid>
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but that's illegal CMake syntax and will cause CMake to complain with "Expression did not evaluate to a known generator expression" or "Expression syntax not recognized."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, more robust 👍
Fixes #2978.
Summary
To understand what I'm trying to fix here, first note:
amrex/Tools/CMake/AMReXFlagsTargets.cmake
Lines 71 to 95 in b3f6738
Then, with Intel's classic compilers, configure AMReX with
-DCMAKE_BUILD_TYPE=Debug
and note the C++ flags listed under "AMReX configuration summary", before and after the changes suggested here. The gist isIntel
as a compiler id was matchingIntelLLVM
(andClang
was also matchingAppleClang
though the flags in that case are the same, so it's harder to tell).EDIT: this actually only affects what's printed in the summary, the flags that are actually used are already correct!
The proposed changes: