[PATCH] D121757: [clang-format] Take out common code for parsing blocks

[Owen Pan via Phabricator via cfe-commits]

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

Because this patch would impact inserting/removing braces, we must test it against a large codebase. Before I landed `RemoveBracesLLVM` and `InsertBraces`, I had tested them with `make check-clang` and compared the result with that of running the same test without the options. I'm not sure if this patch would be worth the effort as the new function only refactored code at two places.



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2716-2717
 
+void UnwrappedLineParser::parseIndentedBlock(bool BracesAreOptional,
+                                             bool RBraceOnSeparateLine) {
+  CompoundStatementIndenter Indenter(this, Style, Line->Level);
----------------
The suggested names are merely my preference.


================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2718-2723
+  CompoundStatementIndenter Indenter(this, Style, Line->Level);
+
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+    FormatToken *LeftBrace = FormatTok;
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2725
+    parseBlock();
+    if (BracesAreOptional && Style.RemoveBracesLLVM) {
+      assert(!NestedTooDeep.empty());
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2736
+
+  if (Style.RemoveBracesLLVM)
+    NestedTooDeep.pop_back();
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2758
 
-  keepAncestorBraces();
-
-  if (FormatTok->is(tok::l_brace)) {
-    FormatToken *LeftBrace = FormatTok;
-    CompoundStatementIndenter Indenter(this, Style, Line->Level);
-    parseBlock();
-    if (Style.RemoveBracesLLVM) {
-      assert(!NestedTooDeep.empty());
-      if (!NestedTooDeep.back())
-        markOptionalBraces(LeftBrace);
-    }
-    addUnwrappedLine();
-  } else {
-    parseUnbracedBody();
-  }
-
-  if (Style.RemoveBracesLLVM)
-    NestedTooDeep.pop_back();
+  parseIndentedBlock(/*BracesAreOptional=*/true);
 }
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2765-2766
 
-  keepAncestorBraces();
-
-  if (FormatTok->is(tok::l_brace)) {
-    CompoundStatementIndenter Indenter(this, Style, Line->Level);
-    parseBlock();
-    if (Style.BraceWrapping.BeforeWhile)
-      addUnwrappedLine();
-  } else {
-    parseUnbracedBody();
-  }
-
-  if (Style.RemoveBracesLLVM)
-    NestedTooDeep.pop_back();
+  parseIndentedBlock(/*BracesAreoptional=*/false,
+                     /*RBraceOnSeparateLine=*/Style.BraceWrapping.BeforeWhile);
 
----------------



================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2839
 
-  keepAncestorBraces();
-
-  if (FormatTok->is(tok::l_brace)) {
-    CompoundStatementIndenter Indenter(this, Style, Line->Level);
-    parseBlock();
-    addUnwrappedLine();
-  } else {
-    addUnwrappedLine();
-    ++Line->Level;
-    parseStructuralElement();
-    --Line->Level;
-  }
-
-  if (Style.RemoveBracesLLVM)
-    NestedTooDeep.pop_back();
+  parseIndentedBlock();
 }
----------------
We can't refactor here as `parseSwitch()` doesn't call `parseUnbracedBody()`, which handles option `InsertBraces`.


================
Comment at: clang/lib/Format/UnwrappedLineParser.h:128
   void parseTryCatch();
+  void parseIndentedBlock(bool BracesAreOptional = true,
+                          bool RBraceOnSeparateLine = true);
----------------
HazardyKnusperkeks wrote:
> I'm no fan of default arguments.
> But we have them all around...
There is no need to have a default for the first parameter now. See line 2839 below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757