[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-06-21 Thread Owen Pan via Phabricator via cfe-commits 
owenpan marked 2 inline comments as done.
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6371-6380
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"

owenpan wrote:
> HazardyKnusperkeks wrote:
> > owenpan wrote:
> > > clang-format breaks the above into two separate sets:
> > > ```
> > > #if FOO
> > > int a = 1;
> > > #else
> > > #endif
> > > #if BAR
> > > int abc = 3;
> > > #else
> > > #endif
> > > ```
> > > and:
> > > ```
> > > #if FOO
> > > #else
> > > int ab = 2;
> > > #endif
> > > #if BAR
> > > #else
> > > int abcd = 4;
> > > #endif
> > > ```
> > > After it finishes with the first set, the preprocessor directives are 
> > > marked as `Finalized`. Then, while the second set is being processed, the 
> > > directives should //not// be skipped when tokens are added to the 
> > > `Change` set. Otherwise, we would end up with the `Change` set below 
> > > without any "scope" context:
> > > ```
> > > int ab = 2;
> > > int abcd = 4;
> > > ```
> > Fascinating. But wouldn't the better fix be that the directives are not 
> > marked as finalized?
> I tried that first, but it broke a lot of unit tests. I'll give it another 
> try when I have time. :)
See D153243.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-09 Thread Owen Pan via Phabricator via cfe-commits 
owenpan marked 2 inline comments as done.
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6371-6380
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"

HazardyKnusperkeks wrote:
> owenpan wrote:
> > clang-format breaks the above into two separate sets:
> > ```
> > #if FOO
> > int a = 1;
> > #else
> > #endif
> > #if BAR
> > int abc = 3;
> > #else
> > #endif
> > ```
> > and:
> > ```
> > #if FOO
> > #else
> > int ab = 2;
> > #endif
> > #if BAR
> > #else
> > int abcd = 4;
> > #endif
> > ```
> > After it finishes with the first set, the preprocessor directives are 
> > marked as `Finalized`. Then, while the second set is being processed, the 
> > directives should //not// be skipped when tokens are added to the `Change` 
> > set. Otherwise, we would end up with the `Change` set below without any 
> > "scope" context:
> > ```
> > int ab = 2;
> > int abcd = 4;
> > ```
> Fascinating. But wouldn't the better fix be that the directives are not 
> marked as finalized?
I tried that first, but it broke a lot of unit tests. I'll give it another try 
when I have time. :)



Comment at: clang/unittests/Format/FormatTestComments.cpp:4319
 /\
-/ 
+/
   )",

HazardyKnusperkeks wrote:
> owenpan wrote:
> > HazardyKnusperkeks wrote:
> > > Unrelated?
> > My editor strips trailing whitespaces on save. I'll leave the fix in 
> > because it's not worth doing it in a separate patch.
> Okay, but that wasn't really trailing, it was part of a string.
Yeah. Fixed in e9acf00. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-09 Thread Björn Schäpers via Phabricator via cfe-commits 
HazardyKnusperkeks added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6371-6380
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"

owenpan wrote:
> clang-format breaks the above into two separate sets:
> ```
> #if FOO
> int a = 1;
> #else
> #endif
> #if BAR
> int abc = 3;
> #else
> #endif
> ```
> and:
> ```
> #if FOO
> #else
> int ab = 2;
> #endif
> #if BAR
> #else
> int abcd = 4;
> #endif
> ```
> After it finishes with the first set, the preprocessor directives are marked 
> as `Finalized`. Then, while the second set is being processed, the directives 
> should //not// be skipped when tokens are added to the `Change` set. 
> Otherwise, we would end up with the `Change` set below without any "scope" 
> context:
> ```
> int ab = 2;
> int abcd = 4;
> ```
Fascinating. But wouldn't the better fix be that the directives are not marked 
as finalized?



Comment at: clang/unittests/Format/FormatTestComments.cpp:4319
 /\
-/ 
+/
   )",

owenpan wrote:
> HazardyKnusperkeks wrote:
> > Unrelated?
> My editor strips trailing whitespaces on save. I'll leave the fix in because 
> it's not worth doing it in a separate patch.
Okay, but that wasn't really trailing, it was part of a string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-08 Thread Owen Pan via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa4c87f8ccacc: [clang-format] Fix consecutive alignments in 
#else blocks (authored by owenpan).

Changed prior to commit:
  https://reviews.llvm.org/D150057?vs=520137&id=520560#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

Files:
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -2759,7 +2759,7 @@
 
   // Checks an edge case in preprocessor handling.
   // These comments should *not* be aligned
-  EXPECT_NE( // change for EQ when fixed
+  EXPECT_EQ(
   "#if FOO\n"
   "#else\n"
   "long a; // Line about a\n"
@@ -4316,7 +4316,7 @@
 )",
 format(R"(//
 /\
-/ 
+/
   )",
getLLVMStyleWithColumns(10)));
 }
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6367,6 +6367,51 @@
"#endif\n"
"}",
Style);
+
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"
+   "#elifdef BAZ\n"
+   "int abcd = 4;\n"
+   "#endif",
+   Style);
+
+  verifyFormat("void f() {\n"
+   "  if (foo) {\n"
+   "#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "bool a = true;\n"
+   "#endif\n"
+   "int abc = 3;\n"
+   "#ifndef BAR\n"
+   "int abcd = 4;\n"
+   "#elif BAZ\n"
+   "bool abcd = true;\n"
+   "#endif\n"
+   "  }\n"
+   "}",
+   Style);
+
+  verifyFormat("void f() {\n"
+   "#if FOO\n"
+   "  a = 1;\n"
+   "#else\n"
+   "  ab = 2;\n"
+   "#endif\n"
+   "}\n"
+   "void g() {\n"
+   "#if BAR\n"
+   "  abc = 3;\n"
+   "#elifndef BAZ\n"
+   "  abcd = 4;\n"
+   "#endif\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -49,8 +49,16 @@
   unsigned Spaces,
   unsigned StartOfTokenColumn,
   bool IsAligned, bool InPPDirective) {
-  if (Tok.Finalized || (Tok.MacroCtx && Tok.MacroCtx->Role == MR_ExpandedArg))
+  auto PPBranchDirectiveStartsLine = [&Tok] {
+return Tok.is(tok::hash) && !Tok.Previous && Tok.Next &&
+   Tok.Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_ifndef,
+ tok::pp_elif, tok::pp_elifdef, tok::pp_elifndef,
+ tok::pp_else, tok::pp_endif);
+  };
+  if ((Tok.Finalized && !PPBranchDirectiveStartsLine()) ||
+  (Tok.MacroCtx && Tok.MacroCtx->Role == MR_ExpandedArg)) {
 return;
+  }
   Tok.setDecision((Newlines > 0) ? FD_Break : FD_Continue);
   Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
Spaces, StartOfTokenColumn, Newlines, "", "",
@@ -522,13 +530,6 @@
? Changes[StartAt].indentAndNestingLevel()
: std::tuple();
 
-  // Keep track if the first token has a non-zero indent and nesting level.
-  // This can happen when aligning the contents of "#else" preprocessor blocks,
-  // which is done separately.
-  bool HasInitialIndentAndNesting =
-  StartAt == 0 &&
-  IndentAndNestingLevel > std::tuple();
-
   // Keep track of the number of commas before the matching tokens, we will only
   // align a sequence of matching tokens if they are preceded by the same number
   // of commas.
@@ -563,19 +564,8 @@
 
   unsigned i = StartAt;
   for (unsigned e = Changes.size(); i != e; ++i) {
-if (Changes[i].indentAndNestingLevel() < IndentAndNestingLevel) {
-  if (!HasInitialIndentAndNesting)
-break;
-  // The contents of preprocessor blocks are aligned separately.
-  // If the initial preprocessor block is indented or nested (e.g. it's in
-  // a function), do not align and exit after

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-08 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6394
+   "#elif BAZ\n"
+   "bool ab = true;\n"
+   "#endif\n"




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added a comment.

In D150057#4325350 , 
@HazardyKnusperkeks wrote:

> I have to say, I don't understand it, but I believe you. Why is continuing, 
> when the token is finalized the right thing?

See D150057#inline-1449546 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-07 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:6371-6380
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"

clang-format breaks the above into two separate sets:
```
#if FOO
int a = 1;
#else
#endif
#if BAR
int abc = 3;
#else
#endif
```
and:
```
#if FOO
#else
int ab = 2;
#endif
#if BAR
#else
int abcd = 4;
#endif
```
After it finishes with the first set, the preprocessor directives are marked as 
`Finalized`. Then, while the second set is being processed, the directives 
should //not// be skipped when tokens are added to the `Change` set. Otherwise, 
we would end up with the `Change` set below without any "scope" context:
```
int ab = 2;
int abcd = 4;
```



Comment at: clang/unittests/Format/FormatTestComments.cpp:4319
 /\
-/ 
+/
   )",

HazardyKnusperkeks wrote:
> Unrelated?
My editor strips trailing whitespaces on save. I'll leave the fix in because 
it's not worth doing it in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-07 Thread Björn Schäpers via Phabricator via cfe-commits 
HazardyKnusperkeks accepted this revision.
HazardyKnusperkeks added a comment.

I have to say, I don't understand it, but I believe you. Why is continuing, 
when the token is finalized the right thing?




Comment at: clang/unittests/Format/FormatTestComments.cpp:4319
 /\
-/ 
+/
   )",

Unrelated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-07 Thread Mitchell via Phabricator via cfe-commits 
mitchell-stellar accepted this revision.
mitchell-stellar added a comment.
This revision is now accepted and ready to land.

LGTM. Well done!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-06 Thread Owen Pan via Phabricator via cfe-commits 
owenpan added inline comments.



Comment at: clang/unittests/Format/FormatTestComments.cpp:2762
   // These comments should *not* be aligned
-  EXPECT_NE( // change for EQ when fixed
+  EXPECT_EQ( // change for EQ when fixed
   "#if FOO\n"

rymiel wrote:
> Should probably get rid of this comment?
Yep! I'll do it before landing. Thanks for catching it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-06 Thread Emilia Kond via Phabricator via cfe-commits 
rymiel added inline comments.



Comment at: clang/unittests/Format/FormatTestComments.cpp:2762
   // These comments should *not* be aligned
-  EXPECT_NE( // change for EQ when fixed
+  EXPECT_EQ( // change for EQ when fixed
   "#if FOO\n"

Should probably get rid of this comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150057

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

[PATCH] D150057: [clang-format] Fix consecutive alignments in #else blocks

2023-05-06 Thread Owen Pan via Phabricator via cfe-commits 
owenpan created this revision.
owenpan added a reviewer: mitchell-stellar.
Herald added projects: All, clang, clang-format.
Herald added a subscriber: cfe-commits.
Herald added reviewers: rymiel, HazardyKnusperkeks, MyDeveloperDay.
owenpan requested review of this revision.

Since 3.8 or earlier, clang-format has been lumping all `#else`, `#elif`, etc 
blocks together when doing whitespace replacements and causing consecutive 
alignments across #else blocks.

Commit c077975 
 partially 
addressed the problem but also triggered "regressions".

This patch fixes the root cause of the problem and "reverts" c077975 
 (except 
for the unit tests).

Fixes https://github.com/llvm/llvm-project/issues/36070.
Fixes https://github.com/llvm/llvm-project/issues/55265.
Fixes https://github.com/llvm/llvm-project/issues/60721.
Fixes https://github.com/llvm/llvm-project/issues/61498.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150057

Files:
  clang/lib/Format/WhitespaceManager.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/FormatTestComments.cpp

Index: clang/unittests/Format/FormatTestComments.cpp
===
--- clang/unittests/Format/FormatTestComments.cpp
+++ clang/unittests/Format/FormatTestComments.cpp
@@ -2759,7 +2759,7 @@
 
   // Checks an edge case in preprocessor handling.
   // These comments should *not* be aligned
-  EXPECT_NE( // change for EQ when fixed
+  EXPECT_EQ( // change for EQ when fixed
   "#if FOO\n"
   "#else\n"
   "long a; // Line about a\n"
@@ -4316,7 +4316,7 @@
 )",
 format(R"(//
 /\
-/ 
+/
   )",
getLLVMStyleWithColumns(10)));
 }
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -6367,6 +6367,51 @@
"#endif\n"
"}",
Style);
+
+  verifyFormat("#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "int ab = 2;\n"
+   "#endif\n"
+   "#ifdef BAR\n"
+   "int abc = 3;\n"
+   "#elifdef BAZ\n"
+   "int abcd = 4;\n"
+   "#endif",
+   Style);
+
+  verifyFormat("void f() {\n"
+   "  if (foo) {\n"
+   "#if FOO\n"
+   "int a = 1;\n"
+   "#else\n"
+   "bool a = true;\n"
+   "#endif\n"
+   "int abc = 3;\n"
+   "#ifndef BAR\n"
+   "int abcd = 4;\n"
+   "#elif BAZ\n"
+   "bool ab = true;\n"
+   "#endif\n"
+   "  }\n"
+   "}",
+   Style);
+
+  verifyFormat("void f() {\n"
+   "#if FOO\n"
+   "  a = 1;\n"
+   "#else\n"
+   "  ab = 2;\n"
+   "#endif\n"
+   "}\n"
+   "void g() {\n"
+   "#if BAR\n"
+   "  abc = 3;\n"
+   "#elifndef BAZ\n"
+   "  abcd = 4;\n"
+   "#endif\n"
+   "}",
+   Style);
 }
 
 TEST_F(FormatTest, FormatHashIfNotAtStartOfLine) {
Index: clang/lib/Format/WhitespaceManager.cpp
===
--- clang/lib/Format/WhitespaceManager.cpp
+++ clang/lib/Format/WhitespaceManager.cpp
@@ -49,8 +49,16 @@
   unsigned Spaces,
   unsigned StartOfTokenColumn,
   bool IsAligned, bool InPPDirective) {
-  if (Tok.Finalized || (Tok.MacroCtx && Tok.MacroCtx->Role == MR_ExpandedArg))
+  auto PPBranchDirectiveStartsLine = [&Tok] {
+return Tok.is(tok::hash) && !Tok.Previous && Tok.Next &&
+   Tok.Next->isOneOf(tok::pp_if, tok::pp_ifdef, tok::pp_ifndef,
+ tok::pp_elif, tok::pp_elifdef, tok::pp_elifndef,
+ tok::pp_else, tok::pp_endif);
+  };
+  if ((Tok.Finalized && !PPBranchDirectiveStartsLine()) ||
+  (Tok.MacroCtx && Tok.MacroCtx->Role == MR_ExpandedArg)) {
 return;
+  }
   Tok.setDecision((Newlines > 0) ? FD_Break : FD_Continue);
   Changes.push_back(Change(Tok, /*CreateReplacement=*/true, Tok.WhitespaceRange,
Spaces, StartOfTokenColumn, Newlines, "", "",
@@ -522,13 +530,6 @@
? Changes[StartAt].indentAndNestingLevel()
: std::tuple();
 
-  // Keep track if the first token has a non-zero indent and nesting level.
-  // This can happen when aligning the contents of "#else" preprocessor blocks,
-  // which is done separately.
-  bool HasI