[PATCH] D95168: [clang-format] Add InsertBraces option

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D95168#3102247 <https://reviews.llvm.org/D95168#3102247>, @owenpan wrote:

> In D95168#3100969 <https://reviews.llvm.org/D95168#3100969>, @MyDeveloperDay wrote:
>
>> In D95168#3099920 <https://reviews.llvm.org/D95168#3099920>, @owenpan wrote:
>>
>>> In D95168#3099739 <https://reviews.llvm.org/D95168#3099739>, @MyDeveloperDay wrote:
>>>
>>>> - 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 <https://reviews.llvm.org/D95168#3039033>, I think it would be better to handle the removal separately. The LLVM Coding Standards has an entire section <https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements> 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();

2. 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;

3. Do we want sub scope to be removed of braces too?

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

4. 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;"
  `


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95168/new/

https://reviews.llvm.org/D95168