[PATCH] D95168: [clang-format] Add Insert/Remove Braces option

[Björn Schäpers via Phabricator via cfe-commits]

HazardyKnusperkeks added inline comments.


================
Comment at: clang/include/clang/Format/Format.h:946
+    /// Remove if there is no comment
+    BIS_RemoveNoComment
+  };
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Maybe differentiate between single line and multi line comments?
> 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
Basically yeah. I for one will only turn on insert, but I think a differentiation here is good.


================
Comment at: clang/include/clang/Format/Format.h:3675
                R.AlwaysBreakTemplateDeclarations &&
            AttributeMacros == R.AttributeMacros &&
            BinPackArguments == R.BinPackArguments &&
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Add it here
> So I notice we don't do this for BraceWrapping, I was going to swing by on this one and see why not.
Most likely a bug.


================
Comment at: clang/include/clang/Format/Format.h:3696
            IndentRequires == R.IndentRequires && IndentWidth == R.IndentWidth &&
-           Language == R.Language &&
+           AutomaticBraces == R.AutomaticBraces && Language == R.Language &&
            IndentWrappedFunctionNames == R.IndentWrappedFunctionNames &&
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Resort ;)
> ouch! thats someone elses bug... but I can look at it.
I wrote that when AutomaticBraces was after Language.

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


================
Comment at: clang/lib/Format/BraceInserter.h:1
+//===--- BraceInserter.h - Format C++ code --------------------------------===//
+//
----------------
MyDeveloperDay wrote:
> HazardyKnusperkeks wrote:
> > Inserter is misleading, if we want it to be able to remove.
> Yes I'm going to rename the files, AutomaticBraces.cpp/.h  AutomaticBracesTests.cpp what do you think?
Sounds good.


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

https://reviews.llvm.org/D95168