Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes

@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
1064

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
3004

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
78

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
1065

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
3735

Resort ;)

clang/lib/Format/BraceInserter.cpp
105 ↗(On Diff #384171)

Remove the {
Oh the irony. :)

clang/lib/Format/BraceInserter.h
1 ↗(On Diff #384171)

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
105 ↗(On Diff #384171)

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?

3695–3696

Add it here

clang/lib/Format/BraceInserter.cpp
21–56 ↗(On Diff #384715)

How about something like that?

114 ↗(On Diff #384715)

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

165 ↗(On Diff #384715)

Is this supposed to be used?

174 ↗(On Diff #384715)

current is not null

178 ↗(On Diff #384715)

!current->Next also holds always

194 ↗(On Diff #384715)

And then this is dead code

211 ↗(On Diff #384715)

Is there always a next?

284 ↗(On Diff #384715)

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

3695–3696

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

3735

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

clang/lib/Format/BraceInserter.h
1 ↗(On Diff #384171)

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.

3695–3696

Most likely a bug.

3735

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
1 ↗(On Diff #384171)

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
21–56 ↗(On Diff #384715)

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
21–56 ↗(On Diff #384715)

Yeah, but I like my variant better. :)

clang/lib/Format/Format.cpp
2461

I think this should keep its brace.

MyDeveloperDay added inline comments.Nov 14 2021, 6:41 AM
clang/lib/Format/BraceInserter.cpp
21–56 ↗(On Diff #384715)

but I'm not clever enough to understand it!