Maxim Kazantsev via llvm-dev
2021-Apr-19 09:43 UTC
[llvm-dev] clang-tidy makes review a pain
Hello everyone, I started noticing that lately we've improved reporting from clang-tidy, pointing out at various formatting issues. However the more verbose it becomes, the more annoyed I feel about it. For example here: https://reviews.llvm.org/D100721 It complains literally about every second line, inserting its comments straight into review. They take as much space as the actual code. Maybe it's just me, but it's really hard to me to understand what the patch is actually doing with so many inlined auto-generated comments. Maybe there is a button to hide them somewhere, but I failed to find it. I understand what was the intention, and clang-tidy is a cool thing in general, but it's getting too intrusive. Does anyone else have the same problem as I do? If there's a lot of people whom it annoys, maybe we should think how to make it less invasive. Maybe it should put these comment when the patch gets approved, or something like this. Thanks, Max -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210419/5bf68d26/attachment.html>
On Mon, Apr 19, 2021 at 11:43 AM Maxim Kazantsev via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Hello everyone, > > > > I started noticing that lately we’ve improved reporting from clang-tidy, > pointing out at various formatting issues. However the more verbose it > becomes, the more annoyed I feel about it. For example here: > > > > https://reviews.llvm.org/D100721 > > > > It complains literally about every second line, inserting its comments > straight into review. They take as much space as the actual code. Maybe > it’s just me, but it’s really hard to me to understand what the patch is > actually doing with so many inlined auto-generated comments. Maybe there is > a button to hide them somewhere, but I failed to find it. > > > > I understand what was the intention, and clang-tidy is a cool thing in > general, but it’s getting too intrusive. Does anyone else have the same > problem as I do? If there’s a lot of people whom it annoys, maybe we should > think how to make it less invasive. Maybe it should put these comment when > the patch gets approved, or something like this. > > > > Thanks, > > Max >I feel the same way about both the clang-tidy and clang-format annotations. They *are* useful, but mainly for the author of the code, not the reviewers. It would be great if clang-tidy/clang-format violations could be communicated out-of-line only. Regards, Nikita -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210419/20409876/attachment.html>
Aaron Ballman via llvm-dev
2021-Apr-19 11:16 UTC
[llvm-dev] clang-tidy makes review a pain
On Mon, Apr 19, 2021 at 5:43 AM Maxim Kazantsev via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > Hello everyone, > > I started noticing that lately we’ve improved reporting from clang-tidy, pointing out at various formatting issues. However the more verbose it becomes, the more annoyed I feel about it. For example here: > > https://reviews.llvm.org/D100721FWIW, all of those diagnostics in the review are correct and are items you should fix. Having an automated tool tell you about them means that reviewers have less work to do and the code base is more likely to end up in a more consistent stylistic state. Having that information earlier rather than later (such as at patch acceptance) means less work for code reviewers, who are sometimes already pretty taxed. That's the whole point to having clang-tidy and clang-format integrated into CI.> It complains literally about every second line, inserting its comments straight into review. They take as much space as the actual code. Maybe it’s just me, but it’s really hard to me to understand what the patch is actually doing with so many inlined auto-generated comments. Maybe there is a button to hide them somewhere, but I failed to find it. > > I understand what was the intention, and clang-tidy is a cool thing in general, but it’s getting too intrusive. Does anyone else have the same problem as I do? If there’s a lot of people whom it annoys, maybe we should think how to make it less invasive. Maybe it should put these comment when the patch gets approved, or something like this.I've complained about the verbosity of clang-tidy and clang-format checks in reviews before, but my concerns come from diagnostics like clang-format not being found on PATH (not something the code author or the reviewer can do anything about), confusion with Phabricator's stacked patches (where clang-tidy will complain about unknown or missing methods that exist in a parent patch but not in the child patch), or clang-format trying to format things it shouldn't (tablegen files, test cases, unit tests, etc). However, as a reviewer, I think the comments in the review you linked are a demonstration of the functionality working as-designed how I'd want it to work. ~Aaron> > > > Thanks, > > Max > > > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Chris Tetreault via llvm-dev
2021-Apr-19 16:31 UTC
[llvm-dev] clang-tidy makes review a pain
I agree that the clang-tidy reporting on your linked patch is quite egregious. I
think that clang-tidy should only post feedback out of line. Ideally, it should
only complain about actual violations of the llvm coding standards. I see
nothing in the coding standards about requiring or preferring that stack locals
being declared const if they can be.
Thanks,
Christopher Tetreault
From: llvm-dev <llvm-dev-bounces at lists.llvm.org> On Behalf Of Maxim
Kazantsev via llvm-dev
Sent: Monday, April 19, 2021 2:43 AM
To: llvm-dev at lists.llvm.org
Subject: [EXT] [llvm-dev] clang-tidy makes review a pain
Hello everyone,
I started noticing that lately we've improved reporting from clang-tidy,
pointing out at various formatting issues. However the more verbose it becomes,
the more annoyed I feel about it. For example here:
https://reviews.llvm.org/D100721
It complains literally about every second line, inserting its comments straight
into review. They take as much space as the actual code. Maybe it's just me,
but it's really hard to me to understand what the patch is actually doing
with so many inlined auto-generated comments. Maybe there is a button to hide
them somewhere, but I failed to find it.
I understand what was the intention, and clang-tidy is a cool thing in general,
but it's getting too intrusive. Does anyone else have the same problem as I
do? If there's a lot of people whom it annoys, maybe we should think how to
make it less invasive. Maybe it should put these comment when the patch gets
approved, or something like this.
Thanks,
Max
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210419/241572bd/attachment.html>
For the record the warning about turning `auto *X` into `const auto *X` shouldn't be emitted, The check was adapted so that warning should no longer be emitted in llvm code. https://github.com/llvm/llvm-project/commit/8a68c40a1bf256523993ee97b39f79001eaade91 I can only guess that the pre-merge build bot is using an old build of clang-tidy as that commit should be in the 11.0.0 release. ~Nathan James On Mon, 2021-04-19 at 09:43 +0000, Maxim Kazantsev via llvm-dev wrote:> Hello everyone, > > I started noticing that lately we’ve improved reporting from clang- > tidy, pointing out at various formatting issues. However the more > verbose it becomes, the more annoyed I feel about it. For example > here: > > https://reviews.llvm.org/D100721 > > It complains literally about every second line, inserting its > comments straight into review. They take as much space as the actual > code. Maybe it’s just me, but it’s really hard to me to understand > what the patch is actually doing with so many inlined auto-generated > comments. Maybe there is a button to hide them somewhere, but I > failed to find it. > > I understand what was the intention, and clang-tidy is a cool thing > in general, but it’s getting too intrusive. Does anyone else have the > same problem as I do? If there’s a lot of people whom it annoys, > maybe we should think how to make it less invasive. Maybe it should > put these comment when the patch gets approved, or something like > this. > > Thanks, > Max > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev