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

Harden cmake regexes when looking for #define strings. #13401

Merged
merged 1 commit into from Feb 18, 2022

Conversation

bangerth
Copy link
Member

Following-up on the comment at #13374 (comment) . Specifically, #define lines don't have to look like

#define FOO

but can be

   #   define FOO

Make sure we find these as well.

I don't think it matters at the moment given that we seem to be finding all of our dependencies just fine. But they might change their style at some future point, and this makes us future proof.

/rebuild

Copy link
Member

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -57,7 +57,7 @@ IF(EXISTS ${ADOLC_SETTINGS_H})
# Check whether ADOL-C is configured with extra trig functions
#
FILE(STRINGS "${ADOLC_SETTINGS_H}" ADOLC_ATRIG_ERF_STRING
REGEX "#define ATRIG_ERF"
REGEX "^[ \t]*#[ \t]*define ATRIG_ERF"
Copy link
Member

@tamiko tamiko Feb 16, 2022

Choose a reason for hiding this comment

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

While at it, would it make sense to change the single space between define and the macro by something like
[ \t]\+ as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, makes sense. So done!

@bangerth
Copy link
Member Author

Seems to work now!

@tamiko tamiko merged commit d7c96aa into dealii:master Feb 18, 2022
@bangerth bangerth deleted the cmake-regex branch February 18, 2022 15:50
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.

None yet

3 participants