[PATCH] D68296: clang-format: Add ability to wrap braces after multi-line control statements

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay requested changes to this revision.
MyDeveloperDay added a comment.
This revision now requires changes to proceed.

Firstly thank you for the patch..

Prior authors have pointed out that new styles need to be supported by a large public style guide as evidence as to why clang-format should support such changes. (if you can find one that points to this being a preference I would reference it here for completeness, see note below)

My personal view is we are getting to the point where so many large projects are already clang-formated to some extent, the definition of the style guide could be based on what clang-format can do and not on necessarily makes the code more readable. What I have begun to notice is some projects on github where clang-format can't support their style simply don't have a .clang-format file. (and that could often be because of 1 or 2 issues which prevent them from adopting it fully.) what is also strange is that that may have a .clang-tidy file which points to them potentially being keen to adopt the clang tools, but why not clang-format when its clearly the simplest to adaopt first)

The reason I ask is to uncover the motivation for adding the change? is it something you need or a nice to have? (adding that to the revision descriptrion helps the reviewers understand the motivation)

We'd normally ask that people be willing to support their revision should there be any need to fix issues, I'd also say that clang-format desperately needs additional developers to help fix issues and in particular to do code reviews,
I only say this because this is seemingly your first revision and whilst I think this revision LGTM I'd hope we could call on you to help maintain clang-format moving forward (given you have an ability to understand how it works in order to do this patch)

Now regarding this revision. It wasn't initially clear what this change gave us, I now I look deeper I see it as being the ability to break the brace when the condition (if,for,catch,etc..) gets itself broken over multiple lines, leading to the view that:

  if (aaaaaaaaaaaaaaaaaaaaa 
      && bbbbbbbbbbbbbbb 
      && cccccccccccccc)
  {
      return true;
  }
  else {
      return false;
  }

is easier to read than:

  if (aaaaaaaaaaaaaaaaaaaaa 
      && bbbbbbbbbbbbbbb 
      && cccccccccccccc) {
      return true;
  }
  else {
      return false;
  }

whilst still recognizing the desire for the compact version when the condition is small.

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

To be honest, I'd be more than happy to see this style as it most closely matches what I used to do all the time before I switched to clang-formatting on save all the time (which meant I couldn't), That was to use a different bracing style based on the complexity of the condition.

I guess my question is do you thin any of the other bracing wrapping rules need the same level of control? (I can't think of any off the top of my head)

Apart from the missing documentation this LGTM (which is why I requested changes), I'm not sure if others have a different view (maybe give them some time to review this), but you'll find it can be quite quiet here. (which is why we need more volunteers to do reviews).

If you can find that style guide evidence!



================
Comment at: clang/include/clang/Format/Format.h:817
+    BWACS_Always
+  };
+
----------------
You are missing a doc change in the clang/docs/ClangStyleOptions.rst file (which I believe you can autogenerate from the Format.h), this needs to be part of the patch


================
Comment at: clang/lib/Format/Format.cpp:643
+                            true};
   switch (Style.BreakBeforeBraces) {
   case FormatStyle::BS_Linux:
----------------
Nit: this a drive by comment, but I feel this is really hard to read.. and not something that needs to be done in this revision but this feels like it needs to be formatted differently

{
/*AfterClass=*/false,
/*AfterEnum=*/false
/*AfterControlStatement=*/FormatStyle::BWACS_Never,
.....
}

because without going back and checking that the AfterControl statement is the 3rd bool I simply can't see what is on and what is off.


Repository:
  rC Clang

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

https://reviews.llvm.org/D68296