[llvm-dev] clang-tidy makes review a pain
Aaron Ballman via llvm-dev
llvm-dev at lists.llvm.org
Tue Apr 20 16:48:41 PDT 2021
On Tue, Apr 20, 2021, 7:34 PM David Blaikie via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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.
>
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
> - 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210420/925eaa41/attachment.html>
More information about the llvm-dev
mailing list