Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

specify C++ language in clang-tidy static-checks.yml #360

Closed
wants to merge 1 commit into from

Conversation

slava77
Copy link
Contributor

@slava77 slava77 commented Dec 30, 2023

it looks like clang-tidy/clang does not recognize quite a few of our files as C++; I propose to add --language=c++ next to -std=c++17 to resolve this

I tested it locally and the error: invalid argument '-std=c++17' not allowed with 'C' appearing while running is gone

@slava77 slava77 requested a review from ariostas December 30, 2023 02:27
@slava77
Copy link
Contributor Author

slava77 commented Dec 30, 2023

I'm curious why are we using clang-format-12 and clang-tidy-12 in the linter instead of what's coming with llvm setup in cmssw on cvmfs (16.0.3 in 13_3 and more recently 17.0.3)?

@ariostas
Copy link
Member

ariostas commented Jan 3, 2024

Hi @slava77, this was part of the reason why I said that the clang-tidy part of the CI is good enough, but definitely not perfect. clang-tidy assumes that .h files (so most of our files) are in C. The thing is that, as far as I can tell, there are only two popular and actively maintained GitHub Actions that run clang-tidy: cpp-linter-action (the one we use) and clang-tidy-review (which I had more issues with). Both of them don't seem to expose a low enough level of control of clang-tidy to fix this sort of things. You can see in this test PR https://github.com/SegmentLinkingTests/TrackLooper/pull/3 that even with that additional flag you still get the same behavior. The purpose of the -std=c++17 flag is to fix some other errors you would get in .cpp files. So I think that the only options would be to either implement a clang-tidy action on our own with the right parameters, try forking one of the existing ones and fix it, or just deal with some fictitious warnings for now until we can test with scram b code-checks. I opted for the last option, but if you prefer we can try the second option.

Regarding version 12 vs 16, I think I accidentally checked the version in CMSSW 13_0_0 instead of 13_3_0, but we can fix that easily.

@slava77
Copy link
Contributor Author

slava77 commented Jan 3, 2024

You can see in this test PR SegmentLinkingTests#3 that even with that additional flag you still get the same behavior.

Uhm, the differences in my local setup were

  • I listed the arguments combined after -- instead of --extra-arg
  • clang-tidy was from the cmssw distribution

I'm curious if one of these is the reason for the failure in SegmentLinkingTests#3

@slava77
Copy link
Contributor Author

slava77 commented Jan 3, 2024

Uhm, the differences in my local setup were

and also I passed the full file, not just the changed lines

@ariostas
Copy link
Member

ariostas commented Jan 3, 2024

I listed the arguments combined after -- instead of --extra-arg

Yeah, that's the issue. I don't know how to pass arguments like that to the action.

@ariostas
Copy link
Member

ariostas commented Jan 3, 2024

and also I passed the full file, not just the changed lines

The action uses the full file, but only reports warnings/errors in lines that were changed.

@slava77
Copy link
Contributor Author

slava77 commented Jan 3, 2024

another option is to not compile .h files, which would work assuming we have them all in .cc files; the errors in .h files can be picked up with -header-filter=SDL/.* or similar

https://stackoverflow.com/questions/47583057/configure-clang-check-for-c-standard-libraries

@ariostas
Copy link
Member

ariostas commented Jan 4, 2024

I finally figured out how to fix this issue. I'll close this PR and tomorrow I'll submit a new PR with all the CI fixes.

@ariostas ariostas closed this Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants