subject:"\[PATCH\] D150403\: \[clang\-format\] Adjust braced list detection \(try 2\)"

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-07-27 Thread Galen Elias via Phabricator via cfe-commits

galenelias added a comment.

In D150403#4539874 , @owenpan wrote:

> This seems to cause a regression. See 
> https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?

I will take a look.  The logic I added is trying to distinguish `{ } {` by 
looking at the token prior to the first l_brace, and checking if it's an 
identifier to quantify this as a braced list brace.  In this case, it's 
actually a `>`, so the code decides it's a scope brace instead.

I could easily patch it to also allow for a `>`, but don't want this to devolve 
into a long trail of hacks either.  Let me think about it a bit more, and I'll 
come up with a forward fix.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-07-27 Thread Owen Pan via Phabricator via cfe-commits

owenpan added a comment.
Herald added a subscriber: wangpc.

This seems to cause a regression. See 
https://github.com/llvm/llvm-project/issues/64134. @galenelias any idea?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-22 Thread Owen Pan via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6dcde658b238: This is a retry of 
https://reviews.llvm.org/D114583, which was backed (authored by galenelias, 
committed by owenpan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1800,6 +1802,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated. It doesn't seem to cause a problem.
+  // So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13732,6 +13732,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-22 Thread Owen Pan via Phabricator via cfe-commits

owenpan accepted this revision.
owenpan added a comment.

In D150403#4362403 , @galenelias 
wrote:

> Galen Elias 
>
> I'm not sure if there is somewhere I should be putting that in the summary or 
> diff itself, or just here in the comments?

Just here in the comments would be enough.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-22 Thread Galen Elias via Phabricator via cfe-commits

galenelias updated this revision to Diff 524484.
galenelias edited the summary of this revision.
galenelias added a comment.

Thanks for the additional review comments.  Hopefully everything if fixed.

In D150403#4358845 , @owenpan wrote:

> Can you put your authorship in the 'John Doe ' 
> 
>  format?

Galen Elias 

I'm not sure if there is somewhere I should be putting that in the summary or 
diff itself, or just here in the comments?


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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated. It doesn't seem to cause a problem.
+  // So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-20 Thread Owen Pan via Phabricator via cfe-commits

owenpan added a comment.

In D150403#4350325 , @galenelias 
wrote:

> In D150403#4347323 , 
> @HazardyKnusperkeks wrote:
>
>> We'll wait a bit, if someone might have a comment. And (at least I) need 
>> name and email for the commit.
>
> Name: Galen Elias
> Email: galenelias at gmail dot com

Can you put your authorship in the 'John Doe ' 

 format?

In D150403#4358562 , @rymiel wrote:

> This patch also fixes https://github.com/llvm/llvm-project/issues/52911 
> (which is probably a duplicate anyway)

@galenelias Can you update the summary?




Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:1789-1790
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated.  It doesn't seem to cause a
+  // problem.  So we only test for the opening braces.
+  auto Tokens = annotate("{\n"




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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-20 Thread Emilia Kond via Phabricator via cfe-commits

rymiel added a comment.

This patch also fixes https://github.com/llvm/llvm-project/issues/52911 (which 
is probably a duplicate anyway)




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:583-584
+  ProbablyBracedList ||
+  NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
+  LBraceStack.back().PrevTok->is(tok::identifier);
 

llvm-project/clang/lib/Format/UnwrappedLineParser.cpp:583:71: warning: '&&' 
within '||' [-Wlogical-op-parentheses]
  NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
  ^~
llvm-project/clang/lib/Format/UnwrappedLineParser.cpp:583:71: note: place 
parentheses around the '&&' expression to silence this warning
  NextTok->is(tok::l_brace) && LBraceStack.back().PrevTok &&
  ^
  (
1 warning generated.



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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-17 Thread Galen Elias via Phabricator via cfe-commits

galenelias added a comment.

In D150403#4347323 , 
@HazardyKnusperkeks wrote:

> We'll wait a bit, if someone might have a comment. And (at least I) need name 
> and email for the commit.

Name: Galen Elias
Email: galenelias at gmail dot com


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-16 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

In D150403#4343708 , @galenelias 
wrote:

> Thanks @HazardyKnusperkeks!  I don't have commit access, so will need someone 
> to land this for me.

We'll wait a bit, if someone might have a comment. And (at least I) need name 
and email for the commit.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-15 Thread Galen Elias via Phabricator via cfe-commits

galenelias added a comment.

Thanks @HazardyKnusperkeks!  I don't have commit access, so will need someone 
to land this for me.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-15 Thread Galen Elias via Phabricator via cfe-commits

galenelias marked 3 inline comments as done.
galenelias added a comment.

Thanks for the review and feedback.  Sorry about the unfortunate timing between 
@sstwcw and my fix - we submitted our fixes less than 24 hours apart.  I didn't 
think there would be another simultaneous fix for this 5 year old bug, so 
didn't even think to link to the review in the GitHub issue.  I will definitely 
do this for any future contributions.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-15 Thread Galen Elias via Phabricator via cfe-commits

galenelias updated this revision to Diff 522245.
galenelias edited the summary of this revision.
galenelias added a comment.

Addressed remaining review feedback.


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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind) << *(FormatTok)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated.  It doesn't seem to cause a
+  // problem.  So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor initialize

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-13 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

Please add the full stop to the code comment(s), and then mark review comments 
as done.

You should also give credit to @sstwcw in the commit message/differential 
description for the token annotator tests.

In D150403#4339530 , @sstwcw wrote:

> I would suggest adding a link to the revision on the issue thread for your 
> future bug fixes.  So people like me don't try to fix what others have fixed.

That's correct, you could even assign the issue to you (or ask to be done so), 
to show that someone is working on it.

Comment at: clang/unittests/Format/TokenAnnotatorTest.cpp:44
+#define EXPECT_BRACE_KIND(FormatTok, Kind) 
\
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)
\

To be consistent, and to get a better output when the expect fails.

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread sstwcw via Phabricator via cfe-commits

sstwcw added a comment.

> To this end, I am tracking the previous token type in a stack parallel
> to LBraceStack to help scope this particular case.

Maybe keep the description up to date with the code.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread sstwcw via Phabricator via cfe-commits

sstwcw added a comment.

I would suggest adding a link to the revision on the issue thread for your 
future bug fixes.  This people like me don't try to fix what others have fixed.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread Galen Elias via Phabricator via cfe-commits

galenelias added a comment.

@HazardyKnusperkeks, thanks for the feedback.  I added the TokenAnnotatorTests 
from @sstwcw's review.  I also updated the design to use a single stack instead 
of the two parallel stacks.


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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread Galen Elias via Phabricator via cfe-commits

galenelias updated this revision to Diff 521828.

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

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  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
@@ -40,6 +40,8 @@
   EXPECT_EQ((FormatTok)->getType(), Type) << *(FormatTok)
 #define EXPECT_TOKEN_PRECEDENCE(FormatTok, Prec)   \
   EXPECT_EQ((FormatTok)->getPrecedence(), Prec) << *(FormatTok)
+#define EXPECT_BRACE_KIND(FormatTok, Kind) \
+  EXPECT_EQ(FormatTok->getBlockKind(), Kind)
 #define EXPECT_TOKEN(FormatTok, Kind, Type)\
   do { \
 EXPECT_TOKEN_KIND(FormatTok, Kind);\
@@ -1783,6 +1785,22 @@
   EXPECT_TOKEN(Tokens[3], tok::colon, TT_CaseLabelColon);
 }
 
+TEST_F(TokenAnnotatorTest, UnderstandsNestedBlocks) {
+  // The closing braces are not annotated.  It doesn't seem to cause a
+  // problem.  So we only test for the opening braces.
+  auto Tokens = annotate("{\n"
+ "  {\n"
+ "{ int a = 0; }\n"
+ "  }\n"
+ "  {}\n"
+ "}");
+  ASSERT_EQ(Tokens.size(), 14u) << Tokens;
+  EXPECT_BRACE_KIND(Tokens[0], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[1], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[2], BK_Block);
+  EXPECT_BRACE_KIND(Tokens[10], BK_Block);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -491,7 +491,11 @@
   // Keep a stack of positions of lbrace tokens. We will
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
-  SmallVector LBraceStack;
+  struct StackEntry {
+FormatToken *Tok;
+const FormatToken *PrevTok;
+  };
+  SmallVector LBraceStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -521,12 +525,12 @@
   } else {
 Tok->setBlockKind(BK_Unknown);
   }
-  LBraceStack.push_back(Tok);
+  LBraceStack.push_back({Tok, PrevTok});
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
 break;
-  if (LBraceStack.back()->is(BK_Unknown)) {
+  if (LBraceStack.back().Tok->is(BK_Unknown)) {
 bool ProbablyBracedList = false;
 if (Style.Language == FormatStyle::LK_Proto) {
   ProbablyBracedList = NextTok->isOneOf(tok::comma, tok::r_square);
@@ -554,7 +558,7 @@
 
   // If we already marked the opening brace as braced list, the closing
   // must also be part of it.
-  ProbablyBracedList = LBraceStack.back()->is(TT_BracedListLBrace);
+  ProbablyBracedList = LBraceStack.back().Tok->is(TT_BracedListLBrace);
 
   ProbablyBracedList = ProbablyBracedList ||
(Style.isJavaScript() &&
@@ -570,8 +574,14 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor initializer list
+  // followed by constructor body, or just adjacent blocks
+  ProbablyBracedList =
+  ProbablyB

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread Björn Schäpers via Phabricator via cfe-commits

HazardyKnusperkeks added a comment.

Shouldn't it have token annotator tests?




Comment at: clang/lib/Format/UnwrappedLineParser.cpp:495-496
   SmallVector LBraceStack;
+  // Track the previous token type corresponding to our lbraces // to help
+  // detect brace types
+  SmallVector PrevTokenKindStack;





Comment at: clang/lib/Format/UnwrappedLineParser.cpp:580-581
+
+  // Distinguish between braced list in a constructor initializer list
+  // followed by constructor body, or just adjacent blocks
+  ProbablyBracedList = ProbablyBracedList ||




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-12 Thread Galen Elias via Phabricator via cfe-commits

galenelias added a subscriber: sstwcw.
galenelias added a comment.

Looks like @sstwcw also submitted a fix for the same issue, with a bit of a 
different approach: https://reviews.llvm.org/D150452


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150403

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

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

2023-05-11 Thread Galen Elias via Phabricator via cfe-commits

galenelias created this revision.
galenelias added reviewers: cpplearner, HazardyKnusperkeks, MyDeveloperDay.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, owenpan.
galenelias requested review of this revision.

This is a retry of https://reviews.llvm.org/D114583, which was backed
out for regressions. This fixes
https://github.com/llvm/llvm-project/issues/33891.

Clang Format is detecting a nested scope followed by another open brace
as a braced initializer list due to incorrectly thinking it's matching a
braced initializer at the end of a constructor initializer list which is
followed by the body open brace.

Unfortunately, UnwrappedLineParser isn't doing a very detailed parse, so
it's not super straightforward to distinguish these cases given the
current structure of calculateBraceTypes. My current hypothesis is that
these can be disambiguated by looking at the token preceding the
l_brace, as initializer list parameters will be preceded by an
identifier, but a scope block generally will not (barring the MACRO
wildcard).

To this end, I am tracking the previous token type in a stack parallel
to LBraceStack to help scope this particular case.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150403

Files:
  clang/lib/Format/UnwrappedLineParser.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+   "  }\n"
+   "}\n"
+   "  }\n"
+   "  {}\n"
+   "}\n"
+   "} // namespace n");
 
   // Long lists should be formatted in columns even if they are nested.
   verifyFormat(
Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -492,6 +492,9 @@
   // update information about whether an lbrace starts a
   // braced init list or a different block during the loop.
   SmallVector LBraceStack;
+  // Track the previous token type corresponding to our lbraces // to help
+  // detect brace types
+  SmallVector PrevTokenKindStack;
   assert(Tok->is(tok::l_brace));
   do {
 // Get next non-comment token.
@@ -522,6 +525,8 @@
 Tok->setBlockKind(BK_Unknown);
   }
   LBraceStack.push_back(Tok);
+  PrevTokenKindStack.push_back(PrevTok ? PrevTok->Tok.getKind()
+   : tok::unknown);
   break;
 case tok::r_brace:
   if (LBraceStack.empty())
@@ -570,8 +575,13 @@
   ProbablyBracedList =
   ProbablyBracedList ||
   NextTok->isOneOf(tok::comma, tok::period, tok::colon,
-   tok::r_paren, tok::r_square, tok::l_brace,
-   tok::ellipsis);
+   tok::r_paren, tok::r_square, tok::ellipsis);
+
+  // Distinguish between braced list in a constructor initializer list
+  // followed by constructor body, or just adjacent blocks
+  ProbablyBracedList = ProbablyBracedList ||
+   NextTok->is(tok::l_brace) &&
+   PrevTokenKindStack.back() == 
tok::identifier;
 
   ProbablyBracedList =
   ProbablyBracedList ||
@@ -602,6 +612,7 @@
 }
   }
   LBraceStack.pop_back();
+  PrevTokenKindStack.pop_back();
   break;
 case tok::identifier:
   if (!Tok->is(TT_StatementMacro))


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -13731,6 +13731,26 @@
"  struct Dummy {};\n"
"  f(v);\n"
"}");
+  verifyFormat("void foo() {\n"
+   "  { // asdf\n"
+   "{ int a; }\n"
+   "  }\n"
+   "  {\n"
+   "{ int b; }\n"
+   "  }\n"
+   "}");
+  verifyFormat("namespace n {\n"
+   "void foo() {\n"
+   "  {\n"
+   "{\n"
+   "  statement();\n"
+   "  if (false) {\n"
+

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

[PATCH] D150403: [clang-format] Adjust braced list detection (try 2)

20 matches

Site Navigation

Mail list logo

Footer information