This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Add Insert/Remove Braces option
AbandonedPublic

Authored by MyDeveloperDay on Jan 21 2021, 1:22 PM.

Details

Summary

InsertBraces will insert braces for single line control flow statements (currently if/else/for).
It currently accepts the following options:

  • Never - Keeps the current behavior and never add braces (also default value)
  • Always - Always add braces to single lines after the control flow statement
  • WrapLikely - If the line after the control flow statement is not braced and likely to wrap, braces will be added
  • Remove - Always remove braces to single line flow control

Diff Detail

Event Timeline

tiagoma requested review of this revision.Jan 21 2021, 1:22 PM
tiagoma created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 1:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tiagoma added a reviewer: curdeius.

There's a clang-tidy check for it.
And clang-format should not, IMO, add not remove tokens but only handle whitespace.

Some background might be useful. I work at Microsoft (Office to be precise). We have our own fork of LLVM internally and I am starting to upstream some of the changes in our fork.

There's a clang-tidy check for it.

Yes - readability-braces-around-statements. I would argue that clang-tidy is a heavyweight tool for this.

And clang-format should not, IMO, add not remove tokens but only handle whitespace.

I don't think token operations are novel, see the InsertTrailingCommas, JavaScriptQuotes, SortUsingDeclarations, FixNamespaceComments options. Also, keep in mind that this option is fairly self contained (it happens before the Formatter pass) and it is off by default.

True, I was aware of the presence of some of these options, thank you for indicating others. I'm not yet entirely convinced, especially that clang-tidy behaviour would be possibly different.

Anyway, a few comments just from the first glance.

clang/docs/ClangFormatStyleOptions.rst
2773

Shouldn't it be consistent with clang-tidy? So instead of an enum, this option might take a number of lines that will trigger brace insertion? None may be sth like -1. "Likely" is very vague.

clang/lib/Format/Format.cpp
1934

Nit: put full stop at the end of the comments.

1948

And make the comments full phrases, starting with a capital letter.

tiagoma added inline comments.Jan 22 2021, 9:56 AM
clang/docs/ClangFormatStyleOptions.rst
2773

I am not sure this is possible. clang-tidy assumes the code is already formatted, we cannot assume that in clang-format. This pass runs before the formatter pass, so basing the behavior on line count would lead to different behavior across runs. For example:

Assuming we can set this option to 2 lines and we have the following code:

if (condition)
	very_long_line_that_will_wrap(arg, arg, arg ,arg)

first run:

if (condition)
	very_long_line_that_will_wrap(
		arg, arg, arg ,arg)

second run:

if (condition) {
	very_long_line_that_will_wrap(
		arg, arg, arg ,arg)}
}

Likely is indeed vague by design. We "guess" that it will wrap in the formatter pass based on the current column value. See Format.cpp @ 1707. What might happen is that the line is incorrectly indentented and the wrapping never actually happens.

lebedev.ri retitled this revision from Add InsertBraces option to [clang-format] Add InsertBraces option.Jan 22 2021, 9:58 AM
tiagoma updated this revision to Diff 318563.Jan 22 2021, 10:09 AM

Update comments

tiagoma marked 2 inline comments as done.Jan 22 2021, 10:09 AM

I think this is one of those reviews that ultimately I think would be useful if we could ensure it works 100% correctly, but I think it goes against the original ethos of clang-format and I think if @djasper or @klimek the original authors were here they'd probably push back.

This reminds me of my own work for a East/West fixer, it proved to be a bit too controversial for some, despite being off by default. but I also agree clang-format has been inserting and moving code around for some time, so its no longer just a whitespace manipulation tool.

I do like this, and I agree clang-tidy is often too heavy weight ( For my source tree which is millions of line of code running clang-tidy over the whole thing is just not possible). I personally run "clang-format -n" over my entire tree every night to sanity check it, this feature would allow me to enforce the use of {} without which I've found over the years is often a source of bugs.

lets see if others have an opinion

MyDeveloperDay added inline comments.Jan 22 2021, 3:55 PM
clang/unittests/Format/FormatTest.cpp
22357

can you use verifyFormat() instead of EXPECT_EQ?

22432

are you testing do/while?

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

Hmm, interesting idea. Do you have anything more precise in mind? Would it be an "augmented" clang-format? I.e. it will have all its options and some additional ones? Or rather more independent tool? Or clang-format's experimental field?

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

Not saying that I'm in favour of creating another tool, but...
I believe that such a tool, if it were pretty much a drop-in replacement of clang-format, it could profit from the current tooling support.
Why? Because most of them let you define the clang-format binary path.

I wonder if we should consider suggesting a different type of tool for clang

clang-reformat

A place where changes such as this and east/west fixer could be actively encouraged.

I don't think this should be done. These kinds of things should be in clang-format. One of the advantages of this and east/west const being in clang-format is that editors, CI systems and other tools have clang-format support. They would be unlikely to get support for a new tool. There are plenty of clang tools which exist but which don't get enough attention to get support in editors CI tools etc.

Not saying that I'm in favour of creating another tool, but...
I believe that such a tool, if it were pretty much a drop-in replacement of clang-format, it could profit from the current tooling support.

If it's a drop in replacement (does everything clang-format does and more), what's the benefit for that cost?

If it's a drop in replacement (does everything clang-format does and more), what's the benefit for that cost?

I'm in agreement here, It was a suggestion just to appease those who don't like clang-format doing such things, but still allowing us to add code changing capabilities. (which I don't understand why that isn't controlled simply by turning the capability off.)

MyDeveloperDay added inline comments.Feb 2 2021, 8:24 AM
clang/unittests/Format/FormatTest.cpp
22340

can these all be verifyFormats

22432

whilst people discuss the ethics of modifying the code ;-)

Can you add some comment based examples

if (condition) // my test
      you_do_you();

if (condition)
      you_do_you(); // my test
MyDeveloperDay added inline comments.Feb 2 2021, 8:27 AM
clang/unittests/Format/FormatTest.cpp
22432

bonus points..

if /*condition*/ (condition) /*condition*/
/*condition*/      you_do_you(); /*condition*/

I'm just going to say this here, but LLVM doesn't like the use of braces on single lines (I don't actually like that myself, but I go with the convention when I remember),

but this is followed only if the reviewer catches it, but by and large its super easy for them to get missed, it took me seconds to find an example..

I would love to see the end for the need for the "elide braces" comment in code reviews, by having an ability to "RemoveBraces"

i.e. this feature should be able to remove the {} in this case

// Defaults that differ when not C++.
if (Language == FormatStyle::LK_TableGen) {
  LLVMStyle.SpacesInContainerLiterals = false;
}
njames93 added inline comments.
clang/unittests/Format/FormatTest.cpp
22432

Should also add test for chained conditionals just to make sure the semantics of the code doesn't change.

if (A)
  if (B)
    callAB();
  else
    callA();
else if (B)
  callB();
else
  call();
tiagoma updated this revision to Diff 321611.Feb 4 2021, 4:28 PM

Add more tests

tiagoma marked 5 inline comments as done.Feb 4 2021, 4:35 PM
tiagoma added inline comments.
clang/unittests/Format/FormatTest.cpp
22432

do/while are not supported in it's current form. We would need to change the logic to add more state. I can have a look at it after this patch is accepted.

22432

This specific test will fail by itself, ie:

StringRef Test = "if /*condition*/ (condition)  /*condition*/\n"
                 "  /*condition*/ you_do_you(); /*condition*/";

verifyFormat(Test);

AFAICT it is because test::messUp causes the 2nd and 3rd comment to stay on the same line. I added a variation of the test.

MyDeveloperDay added inline comments.Feb 5 2021, 3:35 AM
clang/unittests/Format/FormatTest.cpp
22432

The problem with this statement is as soon as you commit, we'd get that defect raised, that is ok its just are you going to stay around long enough to finish it completely? ;-) if not then this puts burden on those who hang out here all the time.

Ideally its probably worth following through with a complete implementation before landing (or as best you can), there is no rush right?

tiagoma updated this revision to Diff 322240.Feb 8 2021, 4:33 PM

Add support for do/while

tiagoma marked an inline comment as done.Feb 9 2021, 10:10 AM
MyDeveloperDay accepted this revision.Feb 13 2021, 7:09 AM

LGTM, we need to run this on a large code base to ensure it doesn't make mistakes in real code, but I think this looks good.

This revision is now accepted and ready to land.Feb 13 2021, 7:09 AM
HazardyKnusperkeks added a project: Restricted Project.May 26 2021, 2:07 AM

Although verifyFormat is nice, I would add some EXPECT_EQ to show that the braces are really inserted.

MyDeveloperDay added inline comments.May 26 2021, 3:04 AM
clang/unittests/Format/FormatTest.cpp
22341

Can you just put the code inline in the verifyFormat, please copy the convention in the file

MyDeveloperDay added inline comments.May 26 2021, 3:06 AM
clang/docs/ClangFormatStyleOptions.rst
2750

I'm not convinced this code has been generated via doc/tools/dump_format_style.py can you confirm this or did you make this change by hand?

could you add tests showing the impact of using [[likely]] and [[unlikely]]

Any thoughts about actually removing braces? eliding braces on single line functions would be very useful for LLVM, its like the most common review comment!

@tiagoma are you still interested in pursuing this? I have some suggestions

  1. I'd like to move the BraceInserter Into its own .cpp and .h files (like I did with the QualifierAlignmentFixer)
  2. I'd like to move the unit tests into their own .cpp file (because I think we need to actually unit tests their functions of BraceInserter more than just testing if via verfiyFormat and I think its cleaner as FormatTest.cpp is very large)
  3. I'd like to see what it would take to remove braces, (eliding the braces on small ifs and control statements is about the number one review comments in LLVM)

I feel like we've general community agreement that clang-format can modify the tokens (following the RFC around QualifierAlignmentFixer) as long as the documentation carries a clear warning and its off by default.

If you are no longer interested in landing this, I am happy to consider taking it on.

MyDeveloperDay added inline comments.Oct 3 2021, 4:23 AM
clang/include/clang/Format/Format.h
998

Please add something like this

/// \warning
///  Setting ``InsertBraces``  to something other than `Never`, COULD
///  lead to incorrect code formatting due to incorrect decisions made due to
///  clang-formats lack of full semantic information.
///  As such extra care should be taken to review code changes made by the use
///  of this option.
/// \endwarning
/// \version 14
clang/lib/Format/Format.cpp
222

I'd like to see "Remove"

MyDeveloperDay added inline comments.Oct 3 2021, 4:26 AM
clang/lib/Format/Format.cpp
2996

Why is this just C++ (LK_Cpp is also Objective C++ and C and Objective C I think)

Why wouldn't I want to add "{}" in C# and/or JavaScript?

clang/unittests/Format/FormatTest.cpp
76

Something here doesn't seem correct, I don't really understand the comment?

MyDeveloperDay requested changes to this revision.Oct 3 2021, 9:31 AM

This patch needs rebasing to main

This revision now requires changes to proceed.Oct 3 2021, 9:31 AM
owenpan added a subscriber: owenpan.Oct 3 2021, 3:23 PM

@tiagoma are you still interested in pursuing this? I have some suggestions

  1. I'd like to move the BraceInserter Into its own .cpp and .h files (like I did with the QualifierAlignmentFixer)
  2. I'd like to move the unit tests into their own .cpp file (because I think we need to actually unit tests their functions of BraceInserter more than just testing if via verfiyFormat and I think its cleaner as FormatTest.cpp is very large)
  3. I'd like to see what it would take to remove braces, (eliding the braces on small ifs and control statements is about the number one review comments in LLVM)

Eliding braces would be much more complicated and should be tackled separately. Below are just some examples:

void f() {
  // Braces can be removed:
  if (a) {
    b;
  } else if (c) {
    d;
  } else {
    e;
  }

  if (a) {
    b;
    c;
  } else if (d) { // Braces may be removed depending on the style.
#undef NDEBUG
    e;
  } else if (g) {
    // comment
  } else {
    {
      h;
    }
  }

  if (a) {
    if (b) { // Braces can be removed.
      c;
    }
  } else if (d) { // Braces may be removed depending on the style.
    e;
  }

  if (a) { // Braces can be removed if the dangling-else warning can be ignored.
    if (b) { // Braces may be removed depending on the style.
      c;
      // comment
    } else if (d) { // Braces may be removed depending on the style.
      e;
    }
  }

  // Braces can be removed:
  if (a) {
    if (b) {
      c;
    }
  }

  // Braces may be removed depending on the style:
  if (a) {
    if (b) {
      c;
    } else {
      d;
    }
  } else {
    e;
  }

  if (a) { // Braces may be removed depending on the style.
    // Braces can be removed:
    if (b) {
      c;
    } else if (d) {
      e;
    }
  } else { // Braces may be removed depending on the style.
    g;
  }

  // Braces can be removed:
  if (a) {
    b;
  } else {
    if (c) {
      d;
    } else {
      e;
    }
  }

I have an experimental implementation that reformats the above to the following (avoiding the dangling-else warning, matching braces of if and else, etc.):

void f() {
  // Braces can be removed:
  if (a)
    b;
  else if (c)
    d;
  else
    e;

  if (a) {
    b;
    c;
  } else if (d) { // Braces may be removed depending on the style.
#undef NDEBUG
    e;
  } else if (g) {
    // comment
  } else {
    { h; }
  }

  if (a) {
    if (b) // Braces can be removed.
      c;
  } else if (d) { // Braces may be removed depending on the style.
    e;
  }

  if (a) { // Braces can be removed if the dangling-else warning can be ignored.
    if (b) { // Braces may be removed depending on the style.
      c;
      // comment
    } else if (d) { // Braces may be removed depending on the style.
      e;
    }
  }

  // Braces can be removed:
  if (a)
    if (b)
      c;

  // Braces may be removed depending on the style:
  if (a)
    if (b)
      c;
    else
      d;
  else
    e;

  if (a) { // Braces may be removed depending on the style.
    // Braces can be removed:
    if (b)
      c;
    else if (d)
      e;
  } else { // Braces may be removed depending on the style.
    g;
  }

  // Braces can be removed:
  if (a)
    b;
  else if (c)
    d;
  else
    e;

@tiagoma are you still interested in pursuing this? I have some suggestions

  1. I'd like to move the BraceInserter Into its own .cpp and .h files (like I did with the QualifierAlignmentFixer)
  2. I'd like to move the unit tests into their own .cpp file (because I think we need to actually unit tests their functions of BraceInserter more than just testing if via verfiyFormat and I think its cleaner as FormatTest.cpp is very large)
  3. I'd like to see what it would take to remove braces, (eliding the braces on small ifs and control statements is about the number one review comments in LLVM)

Eliding braces would be much more complicated and should be tackled separately. Below are just some examples:

I agree with that, to be honest I work in an organization where we'd only ever be inserting them, but I think anyone conforming to LLVM style could appreciate a removal option, but yes lets do that separately.

This will fail in the presence of macro line concatination

if (x)
        return true;
else
        return false;

#define RETURN(x) \
if (x) \
    return true;\
else \
     return false;

it will remove the trailing \

if (x) {
  return true;
} else {
  return false;
}

#define RETURN(x)                                                              \
  if (x) {                                                                     \
    return true;
}
else {
  return false;
}

ping @tiagoma are you still around to pursue this feature request or can this be commandeered?

go for it

@tiagoma to be honest this actually works pretty well, I've been using it in a private build on the large project I work on and I've been ripping through some of the legacy code adding braces like its gone out of fashion, it works a treat, I see only a couple of issues

  1. the macros
  2. nested missing braces
if (x)
  for(int i....)
        foo()

In the later case it will only do the inner for() (I haven't actually tested if I apply it again will it find the outer case)

Its substantially quicker than trying to do the same with clang-tidy, and from my experience with clang-tidy its actually a little more accurate! (sorry just saying!), as often the fix-it get a bit confused.

The recent RFC on the use of clang-format to modify code (https://lists.llvm.org/pipermail/llvm-dev/2021-August/152070.html) means that I think we can move forward with "modifing changes" like this one, where previously I would have been hesitant.

From what I can tell you see, to exclude this from C# and Javascript (but I see no real reason why not do you?)

I'm happy to take a look further if you don't have time (I'll of course keep any credit to you!, this is a great idea).

To confirm

if (x)
    for(int i=0;i<10;i++)
        foo();

will be transformed to

if (x)
  for (int i = 0; i < 10; i++) {
    foo();
  }

but this itself will NOT be transformed to

if (x) {
  for (int i = 0; i < 10; i++) {
    foo();
  }
}

So definitely we are going to miss some cases.

MyDeveloperDay commandeered this revision.Nov 1 2021, 2:09 AM
MyDeveloperDay edited reviewers, added: tiagoma; removed: MyDeveloperDay.
This revision now requires review to proceed.Nov 1 2021, 2:09 AM

I'd like to carry the mantle for this, before making any further changes I'd like to address some of the review comments

  1. Add documentation warning about a modifying change (as agreed by the RFC)
  2. Add version introduced information
  3. Fix issue with Macro continuation (don't add the \n before the } unless you see a \\ line comment, otherwise let clang-format deal with the \n depending on the BraceWrapping style
  4. Add unit tests for Macro continuation and Comment handling
  5. Allow InsertBraces in other languages

TODO:

  • break out into its own file (Format.cpp is getting too big)
  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)
  • @tiagoma please add your name and email addreess to this review so when the time comes to land this I can attribute to you as "Author".

I'd like to carry the mantle for this, before making any further changes I'd like to address some of the review comments

  1. Add documentation warning about a modifying change (as agreed by the RFC)
  2. Add version introduced information
  3. Fix issue with Macro continuation (don't add the \n before the } unless you see a \\ line comment, otherwise let clang-format deal with the \n depending on the BraceWrapping style
  4. Add unit tests for Macro continuation and Comment handling
  5. Allow InsertBraces in other languages

TODO:

  • break out into its own file (Format.cpp is getting too big)

This also holds for FormatTest.cpp.

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later

Yeah we should have that.

  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

Name change is good, the options then also need to be renamed.

  • @tiagoma please add your name and email addreess to this review so when the time comes to land this I can attribute to you as "Author".
clang/include/clang/Format/Format.h
999

Could we just add a big notification on top, that some specially marked options may break the code, and then only mark them, so one does not need to write that much for all possible options?

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

Difficult, yes. But I think it should be in one option.
It hasn't to be implemented everything right from the start. And we could give all those cases a name and a different setting, so that the entire document would be mappable to a .clang-format with a convenience setting for all options at once.

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

I'm thinking more about not adding a "InsertBraces" only later to find it should have been InsertRemoveBraces or AutomaticBraces i.e. I want to have some idea as to how this might work, if it might be possible even if we land separately.

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

Difficult, yes. But I think it should be in one option.
It hasn't to be implemented everything right from the start. And we could give all those cases a name and a different setting, so that the entire document would be mappable to a .clang-format with a convenience setting for all options at once.

What I meant to say is that we should have a separate patch for the removal implementation. I was not opposed to putting both insertion and removal under an enum option like "AutoBraces: Never, Insert, Remove, LLVM, ..."

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

I'm thinking more about not adding a "InsertBraces" only later to find it should have been InsertRemoveBraces or AutomaticBraces i.e. I want to have some idea as to how this might work, if it might be possible even if we land separately.

I think the InsertBraces options can be handled by an enum, but the RemoveBraces options most likely will use a struct. Does it make sense to have both turned on in the same configuration?

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

I'm thinking more about not adding a "InsertBraces" only later to find it should have been InsertRemoveBraces or AutomaticBraces i.e. I want to have some idea as to how this might work, if it might be possible even if we land separately.

I think the InsertBraces options can be handled by an enum, but the RemoveBraces options most likely will use a struct. Does it make sense to have both turned on in the same configuration?

Then this is definitely why we want to think about these now and NOT leave them to a separate review after the Insert case is committed.

From my experience options seem to have a life cycle, it seems many of our options have gone via this route and thinking about this a little more upfront might help us in the long run.

bool -> enum -> struct

I can already envisage some people only wanting inserting of braces on if's and not for's or whiles's (so the options we have are likely not enough as an enum anyway), I think the WrapLikely was the beginning of that.,

If we think remove braces would be a struct then we'd better go for a struct now and not later so there is some symmetry, I'm slightly wondering what would be in the struct? but taking a look at the implementation I've been working on there are a couple of things I noticed already.

  1. Do we want to remove {} from an else or elseif when the if needs braces
if (x==1) {
   foo();
   bar();
}
else  if (x==2)
  baz();
else 
  foo();
  1. Do we want to remove braces from a commented if (this can throw your eye out when looking)
if (x==1)
   // some longish comment
   return x;
  1. Do we want sub scope to be removed of braces too?
if (x==1)
    for (int i=0;i<10;i++)
              foo();
  1. Do we want to prevent the remove for MultiLine scope, where what follows would be spread over multiple lines (a little like the comment really)
    if (x== 1)
        os << "This is a very very very long" 
             << data
             << "More data"
             << "endl;"
`

This is a demo of what I mean D113000: [clang-format] NOT FOR COMMIT - Demo of AutomaticBraces: Remove you can see its pretty aggressive, I could kind of imagine people wanting a little more control

Sometimes this feels inconsistent

and in this case the expression seem so separated from the if() that you might want to keep the {}

Move BraceInserter into its own file and tests

Add experimental support for "Remove"

MyDeveloperDay removed a reviewer: klimek.
MyDeveloperDay set the repository for this revision to rG LLVM Github Monorepo.

Add some more testcases to catch handling removing {} from the nested if, but not the outer if

  • Look further into possible Removal (I have an idea for how this might be possible, and super useful for LLVM where we don't like single if {} ), I'd like to round out on this before introducing the options rather than having to change them later
  • Should we add the possibility of removal should we change the option name to "AutomaticBraces" (thoughts?)

As mentioned in D95168#3039033, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section about this. Some of the listed exceptions/examples there can make things more difficult.

I'm thinking more about not adding a "InsertBraces" only later to find it should have been InsertRemoveBraces or AutomaticBraces i.e. I want to have some idea as to how this might work, if it might be possible even if we land separately.

I think the InsertBraces options can be handled by an enum, but the RemoveBraces options most likely will use a struct. Does it make sense to have both turned on in the same configuration?

I'm in favor of a struct of enums:

AutomaticBraces:
  AfterIf: OnlyIfElse
  AfterElse: Remove #this is obviously only to remove, if the else body is a single line
  AfterWhile: ...

And we can gradually add new enumerators to handle the delicate nuances of adding or removing the braces.
And we should add them one after another.

clang/include/clang/Format/Format.h
3695

Resort ;)

clang/lib/Format/BraceInserter.cpp
106

Remove the {
Oh the irony. :)

clang/lib/Format/BraceInserter.h
2

Inserter is misleading, if we want it to be able to remove.

Then this is definitely why we want to think about these now and NOT leave them to a separate review after the Insert case is committed.

We can just leave a placeholder for the RemoveBraces options and deal with them when we get there. Unless I misunderstood, your suggestion of the AutomaticBraces option (with enum values Insert, Remove, LLVM, etc) would accomplish that.

If we think remove braces would be a struct then we'd better go for a struct now and not later so there is some symmetry, I'm slightly wondering what would be in the struct?

I already had an implementation of RemoveBraces for the LLVM style (minus some exceptions). I started with an enum to accommodate different levels of aggressiveness of removal with LLVM at the minimum end. After looking at several prominent styles, I realized that a struct would work better.

  1. Do we want to remove {} from an else or elseif when the if needs braces
if (x==1) {
   foo();
   bar();
}
else  if (x==2)
  baz();
else 
  foo();
  1. Do we want to remove braces from a commented if (this can throw your eye out when looking)
if (x==1)
   // some longish comment
   return x;
  1. Do we want sub scope to be removed of braces too?
if (x==1)
    for (int i=0;i<10;i++)
              foo();
  1. Do we want to prevent the remove for MultiLine scope, where what follows would be spread over multiple lines (a little like the comment really)
    if (x== 1)
        os << "This is a very very very long" 
             << data
             << "More data"
             << "endl;"
`

I've seen both yes and no for all the above.

I already had an implementation of RemoveBraces for the LLVM style (minus some exceptions).

Why not share the implementation in a review then we can combine them here.

This is a demo of what I mean D113000: [clang-format] NOT FOR COMMIT - Demo of AutomaticBraces: Remove you can see its pretty aggressive, I could kind of imagine people wanting a little more control

Sometimes this feels inconsistent

and in this case the expression seem so separated from the if() that you might want to keep the {}

In my experience, the simplest thing to do is to remove all optional braces as allowed by the language syntax. When you want to add exceptions for matching if-else braces, avoiding dangling-else warnings, etc, things get much more complicated very quickly. :)

I'm in favor of a struct of enums:

AutomaticBraces:
  AfterIf: OnlyIfElse
  AfterElse: Remove #this is obviously only to remove, if the else body is a single line
  AfterWhile: ...

And we can gradually add new enumerators to handle the delicate nuances of adding or removing the braces.
And we should add them one after another.

I like this approach, let me switch over to a struct (that might take me a bit, I'll reorder on the way), I do sort of feel this is for both Inserting/Removing of Braces at the same time should also be the reverse.

i.e. we may not always want to add braces (but allow similar rules to LLVM style)

So a clang-format could be a combination of adding and removing braces depending on the location and usecase, hence in my view why we might as well do this all in one review. I personally wouldn't want 1 implementation for inserting and 1 for removing hence they need to share that configuration.

@tiagoma, I'm going to change the name of the class and file too, just to make that its both Inserting and Removing.

In my experience, the simplest thing to do is to remove all optional braces as allowed by the language syntax. When you want to add exceptions for matching if-else braces, avoiding dangling-else warnings, etc, things get much more complicated very quickly. :)

I can well imagine. I already hit this once where I mutated the TokenAnnotator.cpp incorrect (which caused the unit tests to break). TokenAnnotator.cpp is if/else hell as far as I'm concerned, its a really good test best for some of this stuff!

I already had an implementation of RemoveBraces for the LLVM style (minus some exceptions).

Why not share the implementation in a review then we can combine them here.

I want to try implementing the remaining exceptions for LLVM and tighten some loose ends before submitting the patch. I don't think we should combine Insert and Remove (even if it's technically possible) before each has been tested against a large codebase.

MyDeveloperDay added inline comments.Nov 3 2021, 3:50 AM
clang/lib/Format/BraceInserter.cpp
106

I know!, I'm glad you liked that! (sorry!)

To be honest I can't help myself when I work in LLVM, because this is our company's style, but really this is why I'm keen to add "Remove" support because I need something to keep me honest.

I'm not going to feel that bad, I've run this ability to remove {} over parts of the LLVM code already, its "carnage!!!"

We ALL haven't been keeping a consistent style, My only hope is as we cover some of the LLVM specific cases the amount of churn will reduce (but don't hold your breath!!!)

But I think this is why this is a good idea of @tiagoma , having something like this in clang-format is quite an eye opener. I've already used the Insert ability in our company's legacy code and its great!

MyDeveloperDay retitled this revision from [clang-format] Add InsertBraces option to [clang-format] Add Insert/Remove Braces option.
MyDeveloperDay edited the summary of this revision. (Show Details)

This is very much a WIP, updating the patch (as hit a waypoint), and would like others to have a chance to comment on possible configuration options

AutomaticBraces:
    AfterIf: RemoveNoComment
    AfterElse: RemoveNoComment
    AfterFor: Remove
    AfterWhile: Remove
    AfterDo: Remove

Not completely sure I like the RemoveNoComment, we could handle the non removal if there is a comment case globally (same with the "scope depth") e.g.

And like BreakBeforeBraces I sort of feel that we might want to have some predefines i.e. (LLVM/Custom/Gnu?)

At present its very much Insert or Remove, still to add tests for selective insertion, but I wanted to discuss the configuration options first.

AutomaticBraces: LLVM/Custom
AutomaticBracesOption:
    AfterIf: Remove
    AfterElse: Remove
    AfterFor: Insert
    AfterWhile: Insert
    AfterDo: Insert
    RemovalIfCommentPresent:false
    MaxScopeDepth: 2

RemoveNoComment is the concept of handling a comment after the if() differently

if (x) {
   // comment
   return x;
}
`

I have also run this on some of the sections of LLVM code (Basic/Lexer/Format) to try and catch any potential code change issues.

Also dogfooded it by formatting BraceInserter.cpp.

Not yet done reviewing everything.

clang/include/clang/Format/Format.h
946

Maybe differentiate between single line and multi line comments?

3655

Add it here

clang/lib/Format/BraceInserter.cpp
22–57

How about something like that?

115

(Please read for the comment below.) Could also go into a function and use the same mapping.

166

Is this supposed to be used?

175

current is not null

179

!current->Next also holds always

195

And then this is dead code

212

Is there always a next?

285

This should go into a function, which maybe uses a mapping of the token type to the member used for checking.

MyDeveloperDay marked an inline comment as done.Nov 4 2021, 7:43 AM
MyDeveloperDay added inline comments.
clang/include/clang/Format/Format.h
946

do you think we might want do remove braces from:

if (x) {
  // Remove the braces
  return true;
}

but not from?

if (x) {
  // Don't Remove the braces
  // As the comment it longer
  return true;
}

I could see that

3655

So I notice we don't do this for BraceWrapping, I was going to swing by on this one and see why not.

3695

ouch! thats someone elses bug... but I can look at it.

clang/lib/Format/BraceInserter.h
2

Yes I'm going to rename the files, AutomaticBraces.cpp/.h AutomaticBracesTests.cpp what do you think?

MyDeveloperDay marked an inline comment as done.Nov 4 2021, 7:44 AM
MyDeveloperDay added inline comments.
clang/docs/ClangFormatStyleOptions.rst
1131

Oops missed regenerating this.! (next time around)

clang/include/clang/Format/Format.h
946

Basically yeah. I for one will only turn on insert, but I think a differentiation here is good.

3655

Most likely a bug.

3695

I wrote that when AutomaticBraces was after Language.

Yeah one could and should resort the whole list, but that was not what I meant.

clang/lib/Format/BraceInserter.h
2

Sounds good.

MyDeveloperDay marked an inline comment as done.
MyDeveloperDay edited the summary of this revision. (Show Details)

Add more LLVM rules, and don't remove when there are single or multi lines comments

MyDeveloperDay marked 8 inline comments as done.Nov 13 2021, 5:56 AM
MyDeveloperDay added inline comments.
clang/lib/Format/BraceInserter.cpp
22–57

I think for now

bool SupportsAnyAutomaticBraces(FormatStyle &Style, bool insertion) {
  if (   Style.AutomaticBraces.AfterIf == FormatStyle::BIS_Leave
      && Style.AutomaticBraces.AfterFor == FormatStyle::BIS_Leave
      && Style.AutomaticBraces.AfterWhile == FormatStyle::BIS_Leave
      && Style.AutomaticBraces.AfterDo == FormatStyle::BIS_Leave
      && Style.AutomaticBraces.AfterElse == FormatStyle::BIS_Leave)
      return false;

  if (!insertion){
    if (Style.AutomaticBraces.AfterIf != FormatStyle::BIS_Remove)
    && Style.AutomaticBraces.AfterFor != FormatStyle::BIS_Remove)
    && Style.AutomaticBraces.AfterWhile != FormatStyle::BIS_Remove)
    && Style.AutomaticBraces.AfterDo != FormatStyle::BIS_Remove)
    && Style.AutomaticBraces.AfterElse != FormatStyle::BIS_Remove)
      return false;
  }
  return true;
}

Should cover it.

Should I copy my comments on the old file?

clang/lib/Format/BraceInserter.cpp
22–57

Yeah, but I like my variant better. :)

clang/lib/Format/Format.cpp
2447

I think this should keep its brace.

MyDeveloperDay added inline comments.Nov 14 2021, 6:41 AM
clang/lib/Format/BraceInserter.cpp
22–57

but I'm not clever enough to understand it!