llvm dev - Apr 2021 - clang-tidy makes review a pain

On 19/04/2021 20:32, David Blaikie via llvm-dev wrote:
> 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.
> 
> On Mon, Apr 19, 2021 at 11:12 AM Nathan James via llvm-dev 
> <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> 
>     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
>    
<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
<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 <mailto:llvm-dev at
lists.llvm.org>
>      > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>     <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
>     _______________________________________________
>     LLVM Developers mailing list
>     llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>     <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>

On Tue, Apr 20, 2021 at 3:39 PM Stephen Kelly via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
>
> On 19/04/2021 20:32, David Blaikie via llvm-dev wrote:
> > 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
>
> Thanks,
>
> Stephen.
>
> >
> > On Mon, Apr 19, 2021 at 11:12 AM Nathan James via llvm-dev
> > <llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>> wrote:
> >
> >     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
> >    
<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
<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 <mailto:llvm-dev at
lists.llvm.org>
> >      > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >     <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> >
> >     _______________________________________________
> >     LLVM Developers mailing list
> >     llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
> >     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >     <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> >
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev