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

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

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.

Same.

Regards,
Nikita

Roman

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

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.

--Max

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

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.

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.

--Max

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

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

Generally, if clang-tidy feedback is correct/actionable I think it’s fine - if it’s muddying up the review, then asking the author to address the clang-tidy feedback first, before you review the rest seems like a good approach to me.

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.

If that were the policy, I’d object to it (top level const certainly isn’t done pervasively in LLVM and I’d find it “quirky” (& I do push back on it where it’s not already in use as a micro-convention in some file/area of the code)), but the policy clang-tidy seems to be enforcing is to use “const” on “auto&” and “auto*” where possible, which while not quite written in the LLVM style guide, I think has been discussed/generally encouraged in review - the style guide sort of alludes to it in the example here: https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

Agree with Dave – I generally find that style issues get in the way of reviewing for correctness/efficiency/whatever. I’d much rather have the author clean up the style first, so reviewers don’t have to “claw through” either the style comments or the style issues themselves.

–paulr

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

I see this the different way around. Incorrectly formatted code shows
me that the author has not invested the time to clean up their patch
to make it readable for the reviewer. The formatting standard is to
make the code easier to read which is as important when reviewing as
when looking at the code once committed. As a reviewer, I will remark
anything that seems off, including formatting. And since reformatting
is easy for the author, I think a reviewer can expect these
automatically found items to be addressed before looking at it.

Personally, I add a [style] tag to my reviewer remarks for things that
violate the source formatting style. I.e it's not something that would
block the patch, but should be uncontroversial (e.g. mandated by the
coding standard) and addressed before pushing the patch even if I
accepted the patch already.

Michael

I find the one that remarks that `i` in

for (int i = 0; i < NumThings; ++i) {
   ...

should be uppercase annoying.

Michael

eh, I think that’s probably the wrong direction for LLVM, actually - I think we’ve generally encouraged “const” being explicit when it’s otherwise wrapped up in “auto” - same as for *.

eh, I think that's probably the wrong direction for LLVM, actually - I think we've generally encouraged "const" being explicit when it's otherwise wrapped up in "auto" - same as for *.

Neither make sense to be to be honest. I'd very much like to see clang-tidy in reviews not complain about it. The '*' is quite easy to miss and

     const auto *j1 = getPointer();
     const auto j2 = getPointer();

mean very different things. The latter is also easier to port to a smart (or dumb) pointer.

Thanks,

Stephen.

> eh, I think that's probably the wrong direction for LLVM, actually - I
> think we've generally encouraged "const" being explicit when it's
> otherwise wrapped up in "auto" - same as for *.

Neither make sense to be to be honest. I'd very much like to see
clang-tidy in reviews not complain about it. The '*' is quite easy to
miss and

     const auto *j1 = getPointer();
     const auto j2 = getPointer();

mean very different things.

They do, which to me I think means it's valuable/important to include
both const and * to clarify which thing is intended. It's valuable to
know that something is a pointer - cheap to copy, non-owning (not a
unique_ptr, don't have to use std::move on it), etc. It doesn't mean
every type that is cheap to copy and non-owning is documented in this
way - but does help for some types without making the type name
significantly longer/making expressions more unwieldy, etc.

(I'm surprised there wasn't much more discussion around it (perhaps
there was on an llvm-dev thread or the like) when this rule first went
in: https://github.com/llvm-mirror/llvm/commit/fc9031cdffa3063ef747bd3a98833f164d07fc4a#diff-38d8333325264c104bb94d32db2248c0384fd39d7dbd8512fb4bb4939e3cf2a4
or

The latter is also easier to port to a smart
(or dumb) pointer.

I think it's more important that the code is a bit easier to read than
easier to modify in this way.

- Dave

eh, I think that’s probably the wrong direction for LLVM, actually - I
think we’ve generally encouraged “const” being explicit when it’s
otherwise wrapped up in “auto” - same as for *.

Neither make sense to be to be honest. I’d very much like to see
clang-tidy in reviews not complain about it. The ‘*’ is quite easy to
miss and

const auto *j1 = getPointer();
const auto j2 = getPointer();

mean very different things.

They do, which to me I think means it’s valuable/important to include
both const and * to clarify which thing is intended. It’s valuable to
know that something is a pointer - cheap to copy, non-owning (not a
unique_ptr, don’t have to use std::move on it), etc. It doesn’t mean
every type that is cheap to copy and non-owning is documented in this
way - but does help for some types without making the type name
significantly longer/making expressions more unwieldy, etc.

(I’m surprised there wasn’t much more discussion around it (perhaps
there was on an llvm-dev thread or the like) when this rule first went
in: https://github.com/llvm-mirror/llvm/commit/fc9031cdffa3063ef747bd3a98833f164d07fc4a#diff-38d8333325264c104bb94d32db2248c0384fd39d7dbd8512fb4bb4939e3cf2a4
or

The latter is also easier to port to a smart
(or dumb) pointer.

I think it’s more important that the code is a bit easier to read than
easier to modify in this way.

Strong +1. As a reviewer, I prefer explicit pointer/reference and const qualifiers. Trying to infer that information, especially outside of an IDE, makes reviews significantly harder.

~Aaron

Yes, I know not everyone sees it the same way (even though it's a common point of view outside of llvm somehow), but I just wanted to represent the alternative point of view :).

Also, for clarity, when I referred to "the latter" in my email, I was referring to non-use of both const and *. I just forgot to paste the

  auto j2 = getPointer\(\);

code snippet which would then have been "latter".

Thanks,

Stephen.