Adds clang-tidy to review workflow #2949
Conversation
QuantamHD
force-pushed
the
clang-tidy
branch
2 times, most recently
from
5850890 to
5e65cbb
Compare
|
@maliberty This won't post any comments until it's merged in due to permission issues related to forks which is why it's currently failing. |
This commit will add clang tidy comments to PRs. See https://github.com/marketplace/actions/clang-tidy-review for more examples. Once adopted we should move the action into the OpenROAD Project. Signed-off-by: Ethan Mahintorabi <[email protected]>
QuantamHD
force-pushed
the
clang-tidy
branch
from
5e65cbb to
e34c468
Compare
|
Importantly this clang-tidy only makes comments on modified lines not on the files in general. The action doesn't fail if there are errors it just leaves comments in the PR on how to improve the code. |
|
Example of the output once merged on Openroad. |
|
Most of our code does not comply with the google naming style. This is going to produce tons of warnings on every change. I wouldn't want people to mix-and-match coding styles. We would have to convert modules and then enable it for that module. I don't think we want to just turn this on as is without it being part of a larger plan. Its a substantial effort and disruptive as well. |
|
How about this we split the check into two actions one for all the static analyzers that gets run over the whole project, and one that runs style checking over all modules except sub folders we opt out of? However, this is one of those workflow things that's important to us so we need to make a plan to enable style over the whole project. |
|
I agree it is good to have. Splitting it is fine but I don't see any static analyzers being run here. Is that something you plan to add? I assume that would be more of a nightly thing and not per PR. I don't think we will ever convert abc or sta but the rest of the project is possible (rsz & dbSta are debatable). However the exclude list right now would be nearly everything. Does Google have resources to put behind making this conversion happen? |
|
The following analyzers are running This is definitely a PR thing. Eventually we want to make it a blocking thing. We can opt out abc and sta. Given that they're submodules they won't show up in this check though. It relies on file diffs which don't show up for submodules. We can talk more details, but yes we should be able to allocate resources to this. See uninit'd variable |
|
This PR, in this form, can't be approved now. We should work out a plan for this. @vvbandeira FYI |
Signed-off-by: Ethan Mahintorabi <[email protected]>
|
@maliberty This should be good to go given what we discussed. I plan on adding the identifier naming one in a different PR. |
|
Is it expected that this is failing on this PR @vvbandeira any comments? We should put this in OpenROAD/actions rather than QuantamHD/clang-tidy-review |
|
Yes. It's failing because it's running as a forked workflow which doesn't have permission at the moment. It'll work once merged. I checked in my fork, and it's correctly passing. |
Signed-off-by: Ethan Mahintorabi <[email protected]>
This is the repo in question. I assumed we just keep the repo together for simplicity rather than trying to merge it into a larger one. |
…ady set in the top level CMake Signed-off-by: Ethan Mahintorabi <[email protected]>
|
I have forked the original repo at https://github.com/The-OpenROAD-Project/clang-tidy-review |
hzeller
left a comment
hzeller
left a comment
There was a problem hiding this comment.
Some clang-tidy warnings are a bit overzealous and might trigger on actually desirable code constructs or are sometimes confused.
I am using a fairly aggressive rules in Verible, but have filtered out some that felt not right. Might be a good starting point.
https://github.com/chipsalliance/verible/blob/master/.clang-tidy
…lang tidy setup Signed-off-by: Ethan Mahintorabi <[email protected]>
|
@maliberty This should be ready to go. |
|
lgtm @vvbandeira any final concerns? If not go ahead and merge |
|
@maliberty can you merge now that @vvbandeira has approved |
4 participants
This commit will add clang tidy comments to PRs.
See https://github.com/marketplace/actions/clang-tidy-review for more examples.
Once adopted we should move the action into the OpenROAD Project.