Update rules for clang-tidy and clang --analyze #125
Conversation
nettle
force-pushed
the
0005R-clang-tidy-analyze
branch
from
178240f to
d6b4caa
Compare
There was a problem hiding this comment.
There are plenty of things happening in this patch. Both the changes and the summary feels like "spring cleaning", sweeping the floors and hammering some nails flush if they wiggled themselves out, and also fixing that wall plug that hasn't worked for years.
Can you give me a bulletpoint list that stated what happens and why? From what I gather
- Bash script paramaters have been bazelized,
- Fixing a minor issue with config file inputs when they are from a file group,
- Changing the parameters of the clang call,
- Some code cosmetic update,
- Refactoring the tidy aspect,
- Removing two tests...
And many others. Which parts are NFC, and change for the sake of making the code more readable? Which parts are NFC, but prepare for a later coming feature (like newer bazel versions)? Which parts are functional, as some seem to be functional, and how are they tested?
I approve of most the changes, but the patch in its current state feels like a proof of concept.
| self.grep_file(compile_commands, r"pass\.cc") | ||
| self.grep_file(compile_commands, r"\/gcc") | ||
|
|
||
| def test_bazel_aspect_clang_tidy_pass(self): |
There was a problem hiding this comment.
What is the context behind deleting these tests?
There was a problem hiding this comment.
I think it's because, the aspect used in these tests has been removed, but I'm not sure why it was removed.
There was a problem hiding this comment.
Separate implementation of aspect for clang-tidy is excessive, and we basically do not use it - it is confusing, a bit difficult to use (complex command line) and most importantly it does not support dependencies which makes it useless in some projects.
| self.grep_file(compile_commands, r"pass\.cc") | ||
| self.grep_file(compile_commands, r"\/gcc") | ||
|
|
||
| def test_bazel_aspect_clang_tidy_pass(self): |
There was a problem hiding this comment.
I think it's because, the aspect used in these tests has been removed, but I'm not sure why it was removed.
|
I forgot to add a big summary.
|
nettle
force-pushed
the
0005R-clang-tidy-analyze
branch
from
d6b4caa to
eff03d0
Compare
Why: Bazel rules for clang-tidy and clang --analyze were never updated and never tested on real projects. What: Update rules after testing on some Ericsson projects
nettle
force-pushed
the
0005R-clang-tidy-analyze
branch
from
eff03d0 to
cc7d347
Compare
Done |
Done |
There was a problem hiding this comment.
Thanks for the summary update! As a preface, I agree with the direction and, as much as I understand, with the way you implemented it as well.
I've spent some time with the patch, but I admit that for me, it is simply too big and too complicated to decipher which line changes are attributed to which point in the bulletpoint list. That means I can't do anything beyond a "looks about right" review.
With that said, I'm actually fine with landing the patch as-is, since you are the main maintainer of these rules and I take your word that it satisfies real-world usage needs.
The only issue that remains for me is testing. For the following points:
- Support platform transitions (for multi-platform projects)
I suppose this is borderline impossible to write tests for here.
- Fix plist generation for clang --analyze
What was the specific issue you were experiencing? Can you add tests for it?
- Support config file to accept input from a file_group
Can we add a test for this?
- Collect sources and headers in transitive dependencies
- Simplify launcher scripts, bazelify parameters
- Minor cosmetic changes
- Updated README
Do I understand correctly that all of these are NFC changes?
With tests added and the NFC changes clarified, the patch is ready to land.
3 participants
Why:
Bazel rules for clang-tidy and clang --analyze
were never updated and never tested on real projects.
What: