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

use regex to ignore headers from submodules for clang-tidy #308

Merged
merged 6 commits into from
Nov 17, 2023

Conversation

baperry2
Copy link
Collaborator

Right now we get a ton of clang-tidy warnings from sundials and amrex and just use grep to filter them out, but sometimes things get garbled and that fails.

@baperry2 baperry2 requested a review from jrood-nrel November 15, 2023 21:03
@marchdf
Copy link
Contributor

marchdf commented Nov 16, 2023

I hate to ask but... are you confident this works? I was using this at some point: HeaderFilterRegex: ".+/PeleLMeX/Source/.*\\.H" but I wasn't 100% that it was not excluding stuff I didn't want (I know for sure that mine didn't check the Exec folder). I do think this is worth figuring out so I hope yours works better than mine !

@baperry2
Copy link
Collaborator Author

I did test, but not for Exec. I added that to the regex, as well as dummy code that should introduce warnings with Source, Exec, and PelePhysics to verify that these get caught. Presuming the clang-tidy tests here fail with the correct 3 warnings, I think this is ready to go (after removing the dummy code)

@baperry2 baperry2 requested a review from marchdf November 16, 2023 22:37
@marchdf
Copy link
Contributor

marchdf commented Nov 17, 2023

nicely done! We need this for PeleC as well. Can you remove the grep now from the CI file? Also, does this make things faster?

@baperry2
Copy link
Collaborator Author

There isn't a significant impact on runtime, the main advantage is just getting 200MB of junk out of the log files

The grep is still needed because a handful of warnings still come through from AMReX

PeleLMeX/Submodules/amrex/Src/Extern/SUNDIALS/AMReX_NVector_MultiFab.cpp:586:25: warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

@baperry2 baperry2 enabled auto-merge (squash) November 17, 2023 18:23
@baperry2 baperry2 disabled auto-merge November 17, 2023 18:29
@baperry2 baperry2 enabled auto-merge (squash) November 17, 2023 21:10
@baperry2 baperry2 merged commit 047b533 into AMReX-Combustion:development Nov 17, 2023
20 checks passed
@baperry2 baperry2 deleted the tidy-header-regex branch November 17, 2023 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants