[PATCH] D116316: [clang-format] Add an experimental option to remove optional control statement braces in LLVM C++ code

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3398
 
+**RemoveBracesLLVM** (``Boolean``) :versionbadge:`clang-format 14`
+  Remove optional braces of control statements (``if``, ``else``, ``for``,
----------------
MyDeveloperDay wrote:
> Can we agree on one set of options that can be used for both insertion and removal even if this patch only does removal 
> Can we agree on one set of options that can be used for both insertion and removal even if this patch only does removal 

One idea is to run insertion for all missing braces and then this option to remove the optional ones for LLVM, i.e., an `AutomaticBraces` if you will.


================
Comment at: clang/docs/ClangFormatStyleOptions.rst:3398
 
+**RemoveBracesLLVM** (``Boolean``) :versionbadge:`clang-format 14`
+  Remove optional braces of control statements (``if``, ``else``, ``for``,
----------------
curdeius wrote:
> I don't think it's a good idea to make this option dependent on a particular style. Even if it works for LLVM style only for the moment.
> I don't think it's a good idea to make this option dependent on a particular style. Even if it works for LLVM style only for the moment.

As @MyDeveloperDay said before, eliding braces was (by far?) the most frequent review comments, and the community would appreciate something like this. So it's likely that this option will get used and experimented, and we will get bug reports and other feedback to improve the functionalities. Once we think it's ready, we can easily configure/package the functionalities for other styles.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2338
+  KeepAncestorBraces();
+  NestedTooDeep.push_back(false);
   if (FormatTok->is(tok::l_brace)) {
----------------
HazardyKnusperkeks wrote:
> owenpan wrote:
> > HazardyKnusperkeks wrote:
> > > I think it is worth to create a RAII type for that.
> > Can you explain why an RAII type would be beneficial here?
> You have a lot of push/pop pairs. There is the risk someone (in the future) may misuse it.
I see. I will think about it.


================
Comment at: clang/unittests/Format/FormatTest.cpp:23224
+
+  EXPECT_EQ("if (a)\n"
+            "  if (b)\n"
----------------
MyDeveloperDay wrote:
> any reason these can't be verifyFormats?
Did you mean to add the expected part as a separate case? I don't think it would add any value if there are no braces to remove in the first place?


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

https://reviews.llvm.org/D116316