[llvm-dev] [RFC] Semi-Automatic clang-format of files with low frequency

Tue Jun 30 14:17:55 PDT 2020

For the vote count, I'm also in the group that we should format everything.
Even if you aren't making modifications to the code, it's difficult to read
code you're trying to debug if it's in a different style. Of course, these
formatting changes should go through human review, and we should hold off
on problematic places until we can fix clang-format to deal with it better
(or use tricks like comments for alignment).

For the argument about downstream forks experiencing churn -- is it enough
to say that if your downstream fork hits a merge conflict, just back out of
the merge and clang-format those files in your downstream fork, and your
next attempt to rebase should usually work?

I think posting some clang-formatting patches, and looking at more "bad"
examples might help. There are usually workarounds:

On Tue, Jun 30, 2020 at 1:56 PM Nicolai Hähnle via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> ...

Since you're interested in thoughts about clang-format's capabilities,
> I agree with Matt that the strictness of the approach is a weakness
> that can frequently make code *less* readable. In addition to what he
> mentions, here's an example of a bad change that clang-format wants to
> make that I found after half a minute of scanning our AMDGPU backend
> code:
>
>    // We only support LOAD/STORE and vector manipulation ops for vectors
>    // with > 4 elements.
> -  for (MVT VT : { MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32,
> -                  MVT::v2i64, MVT::v2f64, MVT::v4i16, MVT::v4f16,
> -                  MVT::v4i64, MVT::v4f64, MVT::v8i64, MVT::v8f64,
> -                  MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32 }) {
> +  for (MVT VT :
> +       {MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32, MVT::v2i64,
> +        MVT::v2f64, MVT::v4i16, MVT::v4f16, MVT::v4i64, MVT::v4f64,
> MVT::v8i64,
> +        MVT::v8f64, MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32}) {
>      for (unsigned Op = 0; Op < ISD::BUILTIN_OP_END; ++Op) {
>        switch (Op) {
>
> I don't particularly mind that clang-format puts that initializer list
> onto a new line, or that it changes the whitespace around braces. What
> I do mind: the original code lays the initializer list out carefully
> such that integer and floating point types always come in pairs on the
> same line: v8[if]32 and v16[if]32 on the first line, v2[if]64 and
> v4[if]64 on the second line, and so on. clang-format mindlessly mushes
> the initializer list together in a way that makes it harder to see at
> a glance what is going on.
>

A common strategy for this type of pattern is to use trailing comments for
alignment:

  for (MVT VT : {
           MVT::v8i32, MVT::v8f32, MVT::v16i32, MVT::v16f32,   //
           MVT::v2i64, MVT::v2f64, MVT::v4i16, MVT::v4f16,     //
           MVT::v4i64, MVT::v4f64, MVT::v8i64, MVT::v8f64,     //
           MVT::v16i64, MVT::v16f64, MVT::v32i32, MVT::v32f32  //
       }) {

Live example: https://godbolt.org/z/PehFcC

(( Of course, even better, you can add text to those comments, like "// v8,
v16". But it's a common enough pattern to use empty alignment comments. ))

>
> (Note that this isn't code that I wrote, so I'm not emotionally
> attached to it or anything. But I've run into this kind of problem
> many times during development.)
>
> I believe the common theme here is that clang-format ought to have a
> mode in which it is more accepting of different layouts of lists
> co-existing within the same code base. If that feature was available,
> I would be a strong proponent for adopting it in LLVM itself.
>
Does "// clang-format [ on | off]" suffice as an escape hatch?

>
> Cheers,
> Nicolai
>
>
>
>
>
>
> > I realize it feels like unnecessary churn which is why I suggested this
> be in code which hasn't been touched in some time (I'd be fine if that time
> is 1+ years), but more often than not this is quite small basic style
> issues, similar to the ones below.
> >
> > MyDeveloperDay
> >
> > -  void HandleTranslationUnit(ASTContext& context) override {
> > +  void HandleTranslationUnit(ASTContext &context) override {
> >      if (!Instance.getLangOpts().DelayedTemplateParsing)
> >        return;
> >
> > -      std::set<FunctionDecl*> LateParsedDecls;
> > +      std::set<FunctionDecl *> LateParsedDecls;
> >
> > On Tue, Jun 30, 2020 at 6:55 PM Matt Arsenault <arsenm2 at gmail.com>
> wrote:
> >>
> >>
> >>
> >> On Jun 28, 2020, at 11:30, MyDeveloper Day via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >>
> >> I’m a contributor to clang-format and would like to see LLVM 100% clang
> formatted so we can use LLVM as a massive test-suite for clang-format when
> we make changes.
> >>
> >>
> >>
> >> My main issue with this would be that clang-format does things that I
> don’t believe are stated in the LLVM style guide and I also disagree with.
> There’s a whole set of cases where it makes unwelcome changes to put things
> that were on separate lines on a single line. Whenever I run clang-format,
> I typically git checkout -p to discard all of these.
> >>
> >> For example, it likes to take member initializer lists and pack them
> into as few lines as possible. This is just asking for merge conflicts
> (e.g. AMDGPUSubtarget has a ton of fields in it, and out of tree code is
> constantly adding new fields for unreleased subtargets). It also mangles
> BuildMI calls, where I believe every .add() should be on its own line, and
> stringing this into BuildMI().addFoo().addBar() is way less readable.
> >>
> >> I also believe it’s 4 space indent on line wraps differs from the
> stated 2 space indent level (and this also disagrees with emacs llvm-style)
> >>
> >> -Matt
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> --
> Lerne, wie die Welt wirklich ist,
> aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> 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/20200630/943e1917/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3856 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200630/943e1917/attachment.bin>