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

I 100% get that we might not like the decisions clang-format is making, but
how does one overcome this when adding new code? The pre-merge checks
enforce clang-formatting before commit and that's a common review comment
anyway for those who didn't join the pre-merge checking group. I'm just
wondering are we not all following the same guidelines?

Concerns of clang-format not being good enough are fair enough, but that's
the area I'm focusing my development efforts on (as that's where
I've been
trying to make a small contribution). This effort was driven out of a need
in my view to have an extensive test suite to validate new changes to
clang-format don't break the formatting of pre-formatted code. This
isn't
that possible with LLVM at the moment because large parts are not formatted.

However checking a code base which is in high flux but one that maintains a
100% clang-format clean status, is a near perfect test-suite. Especially
one that I assume will have unit tests for the latest C++ features.

I'm not bored ;-) and whilst I understand this feels somewhat periphery to
the seemingly much more pressing task of developing the next best compiler,
I think we have to remember that clang-format is extensively used. (In my
view by many more people than those actually using clang). GitHub reports
32,000 YAML files with the BasedOnStyle: LLVM alone that is present in the
.clang-format file

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200630/9baa27f6/attachment.html>

On 6/30/20 1:19 PM, MyDeveloper Day via llvm-dev wrote:
> I 100% get that we might not like the decisions clang-format is 
> making, but how does one overcome this when adding new code? The 
> pre-merge checks enforce clang-formatting before commit and that's a 
> common review comment anyway for those who didn't join the pre-merge 
> checking group.

Those checks are advisory. I do not recall a case where 
thoughtfully-hand-formatted code was proposed, adhering to the coding 
guidelines, and a reviewer asked for the file to be run through 
clang-format. When the thoughtfully-formatted code does not match what 
clang-format would produce in such cases, it's generally obvious why. 
Normally, this request is made where the file is inconsistently 
formatted, or formatted in a way that obviously does not comply with our 
guidelines.

I've observed that there is a wide spectrum of developer preferences: 
some thoughtfully and deliberately format all of their code, others deal 
with formatting only when they feel like they absolutely must and are 
happy to have a tool that does anything consistent. Most people are 
probably somewhere in between.

> I'm just wondering are we not all following the same guidelines?

We have specific guidelines that everyone should follow. clang-format 
implements a superset of those.

Here's a counter-proposal: You should hook up a tool that automatically 
opens Phabricator reviews to clang-format the source code. It should do 
this at a very low rate. It may take years to get through everything. 
But, humans will look at the formatting changes, and where clang-format 
should be improved instead of changing the code, we'll discover and 
classify those things. Formatted files can be noted, however, to 
incrementally build the clang-format test suite.

>
> Concerns of clang-format not being good enough are fair enough, but 
> that's the area I'm focusing my development efforts on (as
that's
> where I've been trying to make a small contribution). This effort was 
> driven out of a need in my view to have an extensive test suite to 
> validate new changes to clang-format don't break the formatting of 
> pre-formatted code. This isn't that possible with LLVM at the moment 
> because large parts are not formatted.
>
> However checking a code base which is in high flux but one that 
> maintains a 100% clang-format clean status, is a near perfect 
> test-suite. Especially one that I assume will have unit tests for the 
> latest C++ features.
>
> I'm not bored ;-) and whilst I understand this feels somewhat 
> periphery to the seemingly much more pressing task of developing the 
> next best compiler, I think we have to remember that clang-format is 
> extensively used. (In my view by many more people than those actually 
> using clang). GitHub reports 32,000 YAML files with the BasedOnStyle: 
> LLVM alone that is present in the .clang-format file

Independent of everything else, this is a great success. Maybe we should 
have a website with a tracker on it or something.

Thanks again,

Hal

>
> 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 
> <mailto:arsenm2 at gmail.com>> wrote:
>
>
>
>>     On Jun 28, 2020, at 11:30, MyDeveloper Day via llvm-dev
>>     <llvm-dev at lists.llvm.org <mailto: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
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200630/e53127cd/attachment.html>

On Tue, Jun 30, 2020 at 8:19 PM MyDeveloper Day via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> I 100% get that we might not like the decisions clang-format is making, but
how does one overcome this when adding new code? The pre-merge checks enforce
clang-formatting before commit and that's a common review comment anyway for
those who didn't join the pre-merge checking group. I'm just wondering
are we not all following the same guidelines?
>
> Concerns of clang-format not being good enough are fair enough, but
that's the area I'm focusing my development efforts on (as that's
where I've been trying to make a small contribution). This effort was driven
out of a need in my view to have an extensive test suite to validate new changes
to clang-format don't break the formatting of pre-formatted code. This
isn't that possible with LLVM at the moment because large parts are not
formatted.
>
> However checking a code base which is in high flux but one that maintains a
100% clang-format clean status, is a near perfect test-suite. Especially one
that I assume will have unit tests for the latest C++ features.
>
> I'm not bored ;-) and whilst I understand this feels somewhat periphery
to the seemingly much more pressing task of developing the next best compiler, I
think we have to remember that clang-format is extensively used. (In my view by
many more people than those actually using clang). GitHub reports 32,000 YAML
files with the BasedOnStyle: LLVM alone that is present in the .clang-format
file
My comment there was more targeted against those who would take your
idea and push it to unnecessary and counterproductive extremes. I do
appreciate that you are trying to solve an actual problem _for the
development of clang-format_ :)

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.

(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.

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.

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>

> My comment there was more targeted against those who would take your idea
and push it to unnecessary and counterproductive extremes.
There's nothing counterproductive about just formatting everything and being
done with it. The goal of doing clang-format-diff as we commit code is to
minimize churn so that there isn't one big patch that conflicts with
everything. But if you only format the lines that get touched by a commit, the
work will never be done. At some point you need to finish the work, or the tool
might as well be called clang-make-the-code-inconsistent.
>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.
>
>(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.
>
>Cheers,
>Nicolai
The problem is that while you may find the way it's formatted to be easier
to read, not everybody does. If I were formatting it manually, I probably would
have had 8 lines of pairs, with one pair per line, or I would have had all the
integer types then all the float types if the order is not important. Now we
have a disagreement, and a needless code review issue in the future when
somebody adds a new MVT that needs to be handled and fails to figure out the
pattern. That's the point of the tool. It can't be reasoned with. It
can't be bargained with. It doesn't feel pity of remorse or fear. And it
absolutely will not stop. Ever. Until your code is formatted correctly.

Arguing about code format in code review is a productivity drain. Time spent
figuring out the style of this one random file is a productivity drain. Time
spent inserting a bunch of spaces in a 43 line switch because you added a new
case that is just 1 character too long is a productivity drain. We have a tool
that can do this for us. Maybe it doesn't format everything exactly how
we'd like, but at least you won't have to justify it in code review.
Sure, maybe it will miss things, but at least you don't have to push a new
patch because a line is too long or you put the * next to the type rather than
the variable name. The tool will evolve and improve. Frankly, I'm surprised
that it doesn't already perfectly implement the LLVM coding style, but
surely this is the goal.

From your previous email:
> Hard no on the shorter period. I have changes I'm working on for
several months at a time. 1 year is a minimum for this in my opinion.
Your patch should already be formatted using clang-format-diff, right?
Phabricator is configured to complain on your commit if this is not the case, so
I don't see how a clang-format commit could possibly cause a conflict for
you.

On Tue, Jun 30, 2020 at 11:45 AM Hal Finkel via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> On 6/30/20 1:19 PM, MyDeveloper Day via llvm-dev wrote:
>
> I 100% get that we might not like the decisions clang-format is making,
> but how does one overcome this when adding new code? The pre-merge checks
> enforce clang-formatting before commit and that's a common review
comment
> anyway for those who didn't join the pre-merge checking group.
>
>
> Those checks are advisory. I do not recall a case where
> thoughtfully-hand-formatted code was proposed, adhering to the coding
> guidelines, and a reviewer asked for the file to be run through
> clang-format.
>
In general I'd ask for the magic comment that disables clang-format on the
particular section though, which keeps the file clean with respect to
clang-format.

> When the thoughtfully-formatted code does not match what clang-format
> would produce in such cases, it's generally obvious why. Normally, this
> request is made where the file is inconsistently formatted, or formatted in
> a way that obviously does not comply with our guidelines.
>
I've observed that there is a wide spectrum of developer preferences:
some
> thoughtfully and deliberately format all of their code, others deal with
> formatting only when they feel like they absolutely must and are happy to
> have a tool that does anything consistent. Most people are probably
> somewhere in between.
>
>
> I'm just wondering are we not all following the same guidelines?
>
>
> We have specific guidelines that everyone should follow. clang-format
> implements a superset of those.
>
> Here's a counter-proposal: You should hook up a tool that automatically
> opens Phabricator reviews to clang-format the source code. It should do
> this at a very low rate. It may take years to get through everything. But,
> humans will look at the formatting changes, and where clang-format should
> be improved instead of changing the code, we'll discover and classify
those
> things. Formatted files can be noted, however, to incrementally build the
> clang-format test suite
>
That seems like a great idea actually!

> .
>
>
>
> Concerns of clang-format not being good enough are fair enough, but
that's
> the area I'm focusing my development efforts on (as that's where
I've been
> trying to make a small contribution). This effort was driven out of a need
> in my view to have an extensive test suite to validate new changes to
> clang-format don't break the formatting of pre-formatted code. This
isn't
> that possible with LLVM at the moment because large parts are not
formatted.
>
> However checking a code base which is in high flux but one that maintains
> a 100% clang-format clean status, is a near perfect test-suite. Especially
> one that I assume will have unit tests for the latest C++ features.
>
> I'm not bored ;-) and whilst I understand this feels somewhat periphery
to
> the seemingly much more pressing task of developing the next best compiler,
> I think we have to remember that clang-format is extensively used. (In my
> view by many more people than those actually using clang). GitHub reports
> 32,000 YAML files with the BasedOnStyle: LLVM alone that is present in the
> .clang-format file
>
>
> Independent of everything else, this is a great success. Maybe we should
> have a website with a tracker on it or something.
>
> Thanks again,
>
> Hal
>
>
>
> 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 listllvm-dev at
lists.llvm.orghttps://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
> _______________________________________________
> 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/5c69d36b/attachment.html>