[llvm-dev] clang-tidy makes review a pain

Mon Apr 19 09:31:00 PDT 2021

On Mon, Apr 19, 2021 at 11:51 AM Maxim Kazantsev <mkazantsev at azul.com> wrote:
>
> Putting comments like "you have bad formatting" is off course important. But when I'm reviewing the patches, I'm much more focused on correctness of the logic and its efficiency. And this part is getting much harder because code is flooded with these formatting comments that cannot be disabled.

Shift+A hides them for me, but that's not going to be a particularly
obvious thing for most folks.

> > When working as intended, I don't see it as clutter. It's the exact same amount of comments I would have to manually add to the review myself.
>
> I'm pretty sure that we can talk about formatting/missing "const" keywords once we've understood the code logic and agreed with it. It's a minor technical thing. Understanding logic is a major and complex thing, and it becomes pain with auto-generated flood.
>
> > I don't recall off the top of my head, but don't the automated comments get removed once a subsequent patch is pushed?
>
> No, all comments remain, no matter how much updates you make. They don't get duplicated, but also don't disappear.

Well that's unfortunate and seems like a potentially reasonable
solution to the issue -- if the automated comments are removed when
the patch is updated (and I have no idea how easy/hard/possible that
would be), then the "noise" would go away when the patch author
corrects the issues, and only newly-discovered diagnostics generate
new comments.

I'm not keen on waiting until the end of the review to see those
comments because the comments are not always stylistic (we run a fair
number of correctness clang-tidy checks that are not about style nits)
and getting that feedback early can save debugging time for patch
authors. I'm less worried about when we run clang-format (though it'd
be nice if clang-format were reliable enough for us to commit its
output when the patch is pushed rather than make the patch author do
it as part of the review, then we don't need to see those diagnostics
ever).

~Aaron

>
> --Max
>
> -----Original Message-----
> From: Aaron Ballman <aaron at aaronballman.com>
> Sent: Monday, April 19, 2021 6:38 PM
> To: Maxim Kazantsev <mkazantsev at azul.com>
> Cc: llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] clang-tidy makes review a pain
>
> On Mon, Apr 19, 2021 at 7:25 AM Maxim Kazantsev <mkazantsev at azul.com> wrote:
> >
> > I'm talking about the case when it's working as intended, but because of this, the reviewer has to claw through the debris of these tidy comments trying to pull code pieces together. It's more about perception than about correctness of the automation.
>
> Ah, thank you for clarifying!
>
> When working as intended, I don't see it as clutter. It's the exact same amount of comments I would have to manually add to the review myself. It's a big time-saver for me to say "please address the automated review comments" instead of making those comments myself.
>
> I don't recall off the top of my head, but don't the automated comments get removed once a subsequent patch is pushed? I don't recall seeing duplicated automated comments when a patch is updated without correcting the issues.
>
> ~Aaron
>
> >
> > --Max
> >
> > -----Original Message-----
> > From: Aaron Ballman <aaron at aaronballman.com>
> > Sent: Monday, April 19, 2021 6:17 PM
> > To: Maxim Kazantsev <mkazantsev at azul.com>
> > Cc: llvm-dev at lists.llvm.org
> > Subject: Re: [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/D100721
> >
> > FWIW, 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