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

[sstwcw via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG43c146c96d8e: [clang-format] Take out common code for 
parsing blocks NFC (authored by sstwcw).

Changed prior to commit:
  https://reviews.llvm.org/D121757?vs=420653=426277#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
   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);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2713,55 +2713,49 @@
   } 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);
 }
-addUnwrappedLine();
+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();
+  parseLoopBody(Style.RemoveBracesLLVM, true);
+}
 
-  if (FormatTok->is(tok::l_brace)) {
-CompoundStatementIndenter Indenter(this, Style, Line->Level);
-parseBlock();
-if (Style.BraceWrapping.BeforeWhile)
-  addUnwrappedLine();
-  } else {
-parseUnbracedBody();
-  }
+void UnwrappedLineParser::parseDoWhile() {
+  assert(FormatTok->is(tok::kw_do) && "'do' expected");
+  nextToken();
 
-  if (Style.RemoveBracesLLVM)
-NestedTooDeep.pop_back();
+  parseLoopBody(false, Style.BraceWrapping.BeforeWhile);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
   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);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2713,55 +2713,49 @@
   } 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 

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 420653.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
   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);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2713,6 +2713,29 @@
   } while (!eof());
 }
 
+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 (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2731,43 +2754,14 @@
 parseParens();
   }
 
-  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();
+  parseLoopBody(Style.RemoveBracesLLVM, true);
 }
 
 void UnwrappedLineParser::parseDoWhile() {
   assert(FormatTok->is(tok::kw_do) && "'do' expected");
   nextToken();
 
-  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();
+  parseLoopBody(false, Style.BraceWrapping.BeforeWhile);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
   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);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2713,6 +2713,29 @@
   } while (!eof());
 }
 
+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 (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2731,43 +2754,14 @@
 parseParens();
   }
 
-  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())
-

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

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 420652.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/TokenAnnotator.cpp
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TokenAnnotatorTest.cpp

Index: clang/unittests/Format/TokenAnnotatorTest.cpp
===
--- clang/unittests/Format/TokenAnnotatorTest.cpp
+++ clang/unittests/Format/TokenAnnotatorTest.cpp
@@ -646,6 +646,49 @@
   EXPECT_TOKEN(Tokens[9], tok::l_brace, TT_ObjCBlockLBrace);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsConditionParen) {
+  auto Tokens = annotate("if (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("if constexpr (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("if CONSTEXPR (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[2], tok::l_paren, TT_ConditionLParen);
+
+  // The parentheses following for is not TT_ConditionLParen, because inside is
+  // not just a condition.
+  Tokens = annotate("for (;;) {\n}");
+  ASSERT_EQ(Tokens.size(), 8u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("foreach (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("Q_FOREACH (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("BOOST_FOREACH (Item *item, itemlist) {\n}");
+  ASSERT_EQ(Tokens.size(), 11u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_Unknown);
+  Tokens = annotate("try {\n"
+"} catch (Exception ) {\n"
+"}");
+  ASSERT_EQ(Tokens.size(), 12u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_Unknown);
+
+  Tokens = annotate("while (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+  Tokens = annotate("do {\n} while (true);");
+  ASSERT_EQ(Tokens.size(), 9u) << Tokens;
+  EXPECT_TOKEN(Tokens[4], tok::l_paren, TT_ConditionLParen);
+
+  Tokens = annotate("switch (true) {\n}");
+  ASSERT_EQ(Tokens.size(), 7u) << Tokens;
+  EXPECT_TOKEN(Tokens[1], tok::l_paren, TT_ConditionLParen);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -3177,6 +3177,15 @@
   Style.ColumnLimit =
   20; // to concentrate at brace wrapping, not line wrap due to column limit
 
+  verifyFormat("if (xx\n"
+   ".xx)\n"
+   "  continue;",
+   Style);
+  verifyFormat("while (xx\n"
+   "   .xx)\n"
+   "  continue;",
+   Style);
+
   Style.BraceWrapping.BeforeElse = true;
   EXPECT_EQ(
   "if (foo) {\n"
Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,7 @@
   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);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2422,8 +2422,10 @@
   } else {
 if (FormatTok->isOneOf(tok::kw_constexpr, tok::identifier))
   nextToken();
-if (FormatTok->is(tok::l_paren))
+if (FormatTok->is(tok::l_paren)) {
+  FormatTok->setFinalizedType(TT_ConditionLParen);
   parseParens();
+}
   }
   handleAttributes();
 
@@ -2711,55 +2713,55 @@
   } while (!eof());
 }
 
+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 (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+ 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

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, "");


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 419009.
sstwcw retitled this revision from "[clang-format] Take out common code for 
parsing blocks" to "[clang-format] Take out common code for parsing blocks NFC".

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -123,6 +123,7 @@
   void parseUnbracedBody(bool CheckEOF = false);
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseLoopBody(bool TryRemoveBraces, bool WrapRightBrace);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2699,6 +2699,29 @@
   } while (!eof());
 }
 
+void UnwrappedLineParser::parseLoopBody(bool TryRemoveBraces,
+bool WrapRightBrace) {
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+CompoundStatementIndenter Indenter(this, Style, Line->Level);
+FormatToken *LeftBrace = FormatTok;
+parseBlock();
+if (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2717,43 +2740,15 @@
 parseParens();
   }
 
-  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();
+  parseLoopBody(/*TryRemoveBraces=*/Style.RemoveBracesLLVM,
+/*WrapRightBrace=*/true);
 }
 
 void UnwrappedLineParser::parseDoWhile() {
   assert(FormatTok->is(tok::kw_do) && "'do' expected");
   nextToken();
 
-  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();
+  parseLoopBody(/*BracesAreoptional=*/false, Style.BraceWrapping.BeforeWhile);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -123,6 +123,7 @@
   void parseUnbracedBody(bool CheckEOF = false);
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseLoopBody(bool TryRemoveBraces, bool WrapRightBrace);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2699,6 +2699,29 @@
   } while (!eof());
 }
 
+void UnwrappedLineParser::parseLoopBody(bool TryRemoveBraces,
+bool WrapRightBrace) {
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+CompoundStatementIndenter Indenter(this, Style, Line->Level);
+FormatToken *LeftBrace = FormatTok;
+parseBlock();
+if (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2717,43 +2740,15 @@
 parseParens();
   }
 
-  keepAncestorBraces();
-
-  if 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



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

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



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2743-2744
 
-  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();
+  parseLoopBody(/*TryRemoveBraces=*/Style.RemoveBracesLLVM,
+/*WrapRightBrace=*/true);
 }

Or just the following as they are not default arguments now:
```
  parseLoopBody(Style.RemoveBracesLLVM, true);
```



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2751
 
-  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();
+  parseLoopBody(/*BracesAreoptional=*/false, Style.BraceWrapping.BeforeWhile);
 

Or just the following:
```
  parseLoopBody(false, Style.BraceWrapping.BeforeWhile);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw marked 9 inline comments as done.
sstwcw added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw updated this revision to Diff 418061.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -123,6 +123,7 @@
   void parseUnbracedBody(bool CheckEOF = false);
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseLoopBody(bool TryRemoveBraces, bool WrapRightBrace);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2699,6 +2699,29 @@
   } while (!eof());
 }
 
+void UnwrappedLineParser::parseLoopBody(bool TryRemoveBraces,
+bool WrapRightBrace) {
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+CompoundStatementIndenter Indenter(this, Style, Line->Level);
+FormatToken *LeftBrace = FormatTok;
+parseBlock();
+if (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2717,43 +2740,15 @@
 parseParens();
   }
 
-  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();
+  parseLoopBody(/*TryRemoveBraces=*/Style.RemoveBracesLLVM,
+/*WrapRightBrace=*/true);
 }
 
 void UnwrappedLineParser::parseDoWhile() {
   assert(FormatTok->is(tok::kw_do) && "'do' expected");
   nextToken();
 
-  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();
+  parseLoopBody(/*BracesAreoptional=*/false, Style.BraceWrapping.BeforeWhile);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {


Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -123,6 +123,7 @@
   void parseUnbracedBody(bool CheckEOF = false);
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseLoopBody(bool TryRemoveBraces, bool WrapRightBrace);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2699,6 +2699,29 @@
   } while (!eof());
 }
 
+void UnwrappedLineParser::parseLoopBody(bool TryRemoveBraces,
+bool WrapRightBrace) {
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+CompoundStatementIndenter Indenter(this, Style, Line->Level);
+FormatToken *LeftBrace = FormatTok;
+parseBlock();
+if (TryRemoveBraces) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (WrapRightBrace)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (TryRemoveBraces)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2717,43 +2740,15 @@
 parseParens();
   }
 
-  keepAncestorBraces();
-
-  if (FormatTok->is(tok::l_brace)) {
-FormatToken *LeftBrace = FormatTok;
-CompoundStatementIndenter Indenter(this, Style, Line->Level);
-parseBlock();
-if 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw marked an inline comment as done.
sstwcw added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw marked an inline comment as done.
sstwcw added inline comments.



Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2851
-addUnwrappedLine();
-++Line->Level;
-parseStructuralElement();

HazardyKnusperkeks wrote:
> This is completely missing. Didn't it affect anything?
This chunk of code is already in `parseUnbracedBody`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

HazardyKnusperkeks added a comment.

Looks basically okay.




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:2851
-addUnwrappedLine();
-++Line->Level;
-parseStructuralElement();

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



Comment at: clang/lib/Format/UnwrappedLineParser.h:128
   void parseTryCatch();
+  void parseIndentedBlock(bool BracesAreOptional = true,
+  bool RBraceOnSeparateLine = true);

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121757

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[sstwcw via Phabricator via cfe-commits]

sstwcw created this revision.
sstwcw added a reviewer: clang-format.
sstwcw added a project: clang-format.
Herald added a project: All.
sstwcw requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121757

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/lib/Format/UnwrappedLineParser.h

Index: clang/lib/Format/UnwrappedLineParser.h
===
--- clang/lib/Format/UnwrappedLineParser.h
+++ clang/lib/Format/UnwrappedLineParser.h
@@ -125,6 +125,8 @@
   bool handleCppAttributes();
   FormatToken *parseIfThenElse(IfStmtKind *IfKind, bool KeepBraces = false);
   void parseTryCatch();
+  void parseIndentedBlock(bool BracesAreOptional = true,
+  bool RBraceOnSeparateLine = true);
   void parseForOrWhileLoop();
   void parseDoWhile();
   void parseLabel(bool LeftAlignLabel = false);
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2713,6 +2713,30 @@
   } while (!eof());
 }
 
+void UnwrappedLineParser::parseIndentedBlock(bool BracesAreOptional,
+ bool RBraceOnSeparateLine) {
+  CompoundStatementIndenter Indenter(this, Style, Line->Level);
+
+  keepAncestorBraces();
+
+  if (FormatTok->is(tok::l_brace)) {
+FormatToken *LeftBrace = FormatTok;
+parseBlock();
+if (BracesAreOptional && Style.RemoveBracesLLVM) {
+  assert(!NestedTooDeep.empty());
+  if (!NestedTooDeep.back())
+markOptionalBraces(LeftBrace);
+}
+if (RBraceOnSeparateLine)
+  addUnwrappedLine();
+  } else {
+parseUnbracedBody();
+  }
+
+  if (Style.RemoveBracesLLVM)
+NestedTooDeep.pop_back();
+}
+
 void UnwrappedLineParser::parseForOrWhileLoop() {
   assert(FormatTok->isOneOf(tok::kw_for, tok::kw_while, TT_ForEachMacro) &&
  "'for', 'while' or foreach macro expected");
@@ -2731,43 +2755,15 @@
 parseParens();
   }
 
-  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);
 }
 
 void UnwrappedLineParser::parseDoWhile() {
   assert(FormatTok->is(tok::kw_do) && "'do' expected");
   nextToken();
 
-  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);
 
   // FIXME: Add error handling.
   if (!FormatTok->is(tok::kw_while)) {
@@ -2840,21 +2836,7 @@
 parseParens();
   }
 
-  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();
 }
 
 // Operators that can follow a C variable.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits