[clang-format] Take out common code for parsing blocks NFC

[HazardyKnusperkeks]

Looks basically okay.

clang/lib/Format/UnwrappedLineParser.cpp

2843

This is completely missing. Didn't it affect anything?

clang/lib/Format/UnwrappedLineParser.h

128

I'm no fan of default arguments.
But we have them all around...

[MyDeveloperDay]

Can you add a summary as to why you are making this change? the code doesn't look the same before/after so whats changing and why? is there no unit tests that perhaps need to be added?

[sstwcw]

clang/lib/Format/UnwrappedLineParser.cpp

2843

This chunk of code is already in parseUnbracedBody.

[owenpan]

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.

clang/lib/Format/UnwrappedLineParser.cpp

2716–2717

The suggested names are merely my preference.

2718–2723

2725

2736

2736

2758–2759

2843

We can't refactor here as parseSwitch() doesn't call parseUnbracedBody(), which handles option InsertBraces.

clang/lib/Format/UnwrappedLineParser.h

128

There is no need to have a default for the first parameter now. See line 2839 below.

[sstwcw]

This patch is only intended to reduce the number of times the functionality gets implemented separately. Any change in behavior would be unintended. And we also use the parseIndentedBlock in Verilog stuff, so it's not just two places. I will wait for the check-clang result to see what to do.

[MyDeveloperDay]

Just an observation, I have this in my Day Job all the time..

1. Developer A develops a piece of code
2. Over the years the developers in our group learn that code and become familiar with it
3. Developer B arrives in our group, telling us we are doing it all wrong we don't understand, but they are going to refactor it for us and correct the error of our ways.
4. Now now one in our group understands the code, except Developer B
5. Developer B gets annoyed at our lack of understanding of his/her more masterful refactoring skills and disappears...
6. No one understands the code and everyone has to relearn the code. (Developer B's name is now mud!)

Refactoring is an important step, but in my view it shouldn't be the first contribution someone makes. I understand it you think its better, it possibly is.. but those of us who have been in here for years now no longer recognize the code and have to read it with a fine tooth combe again.

I personally don't see the code as being functional equal before or after, and I've kicked myself for not being more thorough in review when someone does this..that's probably my lack of understanding.

If you are fixing a bug, then ok, lets look at it, but just because you don't like the way it looks or you think there is repetition. If your an experienced contributor then maybe you'd have our trust...but otherwise we need to express caution here in my view. Just incase you chose to up and disappear.

Refactoring yes, but if its "Drive by" Refactoring, I'm not so keen.

[sstwcw]

For the new stuff I have the option of still adding the function parseIndentedBlock but only using it in new code. Please be more blunt about whether I should close this revision and do it that way. I guess I might have misunderstood you before from how you reacted when I closed the large patch.

[MyDeveloperDay]

In D121757#3389218, @sstwcw wrote:

For the new stuff I have the option of still adding the function parseIndentedBlock but only using it in new code. Please be more blunt about whether I should close this revision and do it that way. I guess I might have misunderstood you before from how you reacted when I closed the large patch.

Ok being blunt.. the things you are asking to do, look noble, I just don't always understand the motivation? you are seemingly not fixing anything, you are not adding new tests, its just moving things around, but I don't know your commitment yet? are you hanging around? are you going to fix bugs?

I feel like the guidance for new developers is do a "good first issue", the reviewers have triaged the clang-format issues to try and identify what makes a good first issue for a new contributor

https://github.com/llvm/llvm-project/issues?q=is%3Aopen+label%3A%22good+first+issue%22+label%3Aclang-format

Ultimately if you don't have commit access your going to ask one of us to land this for you, as such we might need persuading, Certainly for me to do that for you I need to see a benefit otherwise its just change in my view.

[owenpan]

In D121757#3389092, @sstwcw wrote:

This patch is only intended to reduce the number of times the functionality gets implemented separately. Any change in behavior would be unintended. And we also use the parseIndentedBlock in Verilog stuff, so it's not just two places. I will wait for the check-clang result to see what to do.

In that case, I can accept this patch if the review comments are addressed. We can add inserting braces to switch in a separate patch. Not sure what name we should use for the function though as it would not be parsing the body of loop statements anymore.

[sstwcw]

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

[owenpan]

clang/lib/Format/UnwrappedLineParser.cpp

2721–2722

You swapped these two lines. We want to remember the l_brace before calling other functions in case they update FormatTok.

2736–2737

Or just the following as they are not default arguments now:

parseLoopBody(Style.RemoveBracesLLVM, true);

2758

Or just the following:

parseLoopBody(false, Style.BraceWrapping.BeforeWhile);

[owenpan]

In D121757#3406814, @sstwcw wrote:

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

To test a token-changing patch (e.g. inserting/removing braces) on a large codebase like clang, I would go through the following steps:

1. Build a debug version of clang-format from scratch without applying the patch.
2. Run the built clang-format on every .(c|cpp|h) file under clang/.
3. Build clang.
4. Run check-clang and save the output in a log file.
5. Run git reset --hard and apply the patch.
6. Build clang-format again.
7. Add InsertBraces: true to clang/.clang-format.
8. Repeat steps 2-4 above.
9. Compare the new log file with the pre-patch one. They should have exactly the same Unresolved/Failed tests. They should also have the same numbers of Skipped/Unsupported/Passed/Expectedly Failed tests.

[owenpan]

In D121757#3407539, @owenpan wrote:

In D121757#3406814, @sstwcw wrote:

I tried formatting the files in clang-formatted-files.txt. Besides the files in the list that get changed when formatted with the program built from main, none gets changed when I format them with the program built from this patch, whether or not parseSwitch is modified.

To test a token-changing patch (e.g. inserting/removing braces) on a large codebase like clang, I would go through the following steps:

1. Build a debug version of clang-format from scratch without applying the patch.
2. Run the built clang-format on every .(c|cpp|h) file under clang/.
3. Build clang.
4. Run check-clang and save the output in a log file.
5. Run git reset --hard and apply the patch.
6. Build clang-format again.

And then run git reset --hard again before step 7 below.

7. Add InsertBraces: true to clang/.clang-format.
8. Repeat steps 2-4 above.
9. Compare the new log file with the pre-patch one. They should have exactly the same Unresolved/Failed tests. They should also have the same numbers of Skipped/Unsupported/Passed/Expectedly Failed tests.

[sstwcw]

I ran check-clang after formatting the entire code base with the new version. It turned out it did break some tests. It seems to be because it messed up these comments.

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
index 7a8d756b09fd..53b2ddd12885 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
@@ -33,8 +33,8 @@ void f(int n) {
   case 6:
     do {
       [[fallthrough]];
-    } while (
-        false); // expected-error {{does not directly precede switch label}}
+    } while (false); // expected-error {{does not directly precede switch
+                     // label}}
   case 7:
     switch (n) {
     case 0:

Now I have undone the parts that changed this. Formatting with the new version yields the same result as formatting with the program built from the main branch, whether with or without the InsertBraces option, except for this one line:

diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 4c6b6da9e415..4d063378f37c 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1115,7 +1115,7 @@ static_assert(check(S(5), 11), "");
 namespace PR14171 {
 
 struct X {
-  constexpr(operator int)() const { return 0; }
+  constexpr (operator int)() const { return 0; }
 };
 static_assert(X() == 0, "");

[owenpan]

In D121757#3415402, @sstwcw wrote:

I ran check-clang after formatting the entire code base with the new version. It turned out it did break some tests. It seems to be because it messed up these comments.

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
index 7a8d756b09fd..53b2ddd12885 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.fallthrough/p1.cpp
@@ -33,8 +33,8 @@ void f(int n) {
   case 6:
     do {
       [[fallthrough]];
-    } while (
-        false); // expected-error {{does not directly precede switch label}}
+    } while (false); // expected-error {{does not directly precede switch
+                     // label}}
   case 7:
     switch (n) {
     case 0:

Now I have undone the parts that changed this. Formatting with the new version yields the same result as formatting with the program built from the main branch, whether with or without the InsertBraces option, except for this one line:

diff --git a/clang/test/SemaCXX/constant-expression-cxx11.cpp b/clang/test/SemaCXX/constant-expression-cxx11.cpp
index 4c6b6da9e415..4d063378f37c 100644
--- a/clang/test/SemaCXX/constant-expression-cxx11.cpp
+++ b/clang/test/SemaCXX/constant-expression-cxx11.cpp
@@ -1115,7 +1115,7 @@ static_assert(check(S(5), 11), "");
 namespace PR14171 {
 
 struct X {
-  constexpr(operator int)() const { return 0; }
+  constexpr (operator int)() const { return 0; }
 };
 static_assert(X() == 0, "");

Thanks for running check-clang. Can you address the review comments so that we can accept this patch? After you land it, I will add handling switch to InsertBraces and run check-clang again.

[owenpan]

LGTM

[sstwcw]

Would you have a look at the parent revision namely D121756?

clang/lib/Format/UnwrappedLineParser.h

private:
   void parseParens(TokenType AmpAmpTokenType = TT_Unknown);
   void parseSquare(bool LambdaIntroducer = false);
   void keepAncestorBraces();
   void parseUnbracedBody(bool CheckEOF = false);
   void handleAttributes();
   bool handleCppAttributes();
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseLoopBody(bool TryRemoveBraces, bool WrapRightBrace);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
   void parseCaseLabel();
   void parseSwitch();
   void parseNamespace();
   void parseModuleImport();
   void parseNew();

clang/lib/Format/UnwrappedLineParser.cpp

if (FormatTok->is(tok::l_paren)) {
       if (FormatTok->is(tok::l_brace))
         parseChildBlock();
       return;
     }
     nextToken();
   } while (!eof());
 }
 
-void UnwrappedLineParser::parseForOrWhileLoop() {
-  assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
-         "'for', 'while' or foreach macro expected");
-  nextToken();
-  // JS' for await ( ...
-  if (Style.isJavaScript() && FormatTok->is(Keywords.kw_await))
-    nextToken();
-  if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
-    nextToken();
-  if (FormatTok->is(tok::l_paren))
-    parseParens();
-
+void UnwrappedLineParser::parseLoopBody(bool TryRemoveBraces,
+                                        bool WrapRightBrace) {
   keepAncestorBraces();
 
   if (FormatTok->is(tok::l_brace)) {
     FormatToken *LeftBrace = FormatTok;
     CompoundStatementIndenter Indenter(this, Style, Line->Level);
     parseBlock();
-    if (Style.RemoveBracesLLVM) {
+    if (TryRemoveBraces) {
       assert(!NestedTooDeep.empty());
       if (!NestedTooDeep.back())
         markOptionalBraces(LeftBrace);
     }
+    if (WrapRightBrace)
       addUnwrappedLine();
   } else {
     parseUnbracedBody();
   }
 
-  if (Style.RemoveBracesLLVM)
+  if (TryRemoveBraces)
     NestedTooDeep.pop_back();
 }
 
-void UnwrappedLineParser::parseDoWhile() {
-  assert(FormatTok->is(tok::kw_do) && "'do' expected");
+void UnwrappedLineParser::parseForOrWhileLoop() {
+  assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
+         "'for', 'while' or foreach macro expected");
   nextToken();
+  // JS' for await ( ...
+  if (Style.isJavaScript() && FormatTok->is(Keywords.kw_await))
+    nextToken();
+  if (Style.isCpp() && FormatTok->is(tok::kw_co_await))
+    nextToken();
+  if (FormatTok->is(tok::l_paren))
+    parseParens();
 
-  keepAncestorBraces();
-
-  if (FormatTok->is(tok::l_brace)) {
-    CompoundStatementIndenter Indenter(this, Style, Line->Level);
-    parseBlock();
-    if (Style.BraceWrapping.BeforeWhile)
-      addUnwrappedLine();
-  } else {
-    parseUnbracedBody();
-  }
+  parseLoopBody(Style.RemoveBracesLLVM, true);
 
-  if (Style.RemoveBracesLLVM)
-    NestedTooDeep.pop_back();
+void UnwrappedLineParser::parseDoWhile() {
+  assert(FormatTok->is(tok::kw_do) && "'do' expected");
+  nextToken();
+
+  parseLoopBody(false, Style.BraceWrapping.BeforeWhile);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {
     addUnwrappedLine();
     return;
   }
 
   // If in Whitesmiths mode, the line with the while() needs to be indented
   // to the same level as the block.
void UnwrappedLineParser::parseSwitch() {
   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();
 }
 
 // Operators that can follow a C variable.
 static bool isCOperatorFollowingVar(tok::TokenKind kind) {
   switch (kind) {
   case tok::ampamp:
   case tok::ampequal:
   case tok::arrow: