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

[owenpan]

Summary

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.

[rymiel]

clang/unittests/Format/FormatTestComments.cpp

2762

Should probably get rid of this comment?

[owenpan]

clang/unittests/Format/FormatTestComments.cpp

2762

Yep! I'll do it before landing. Thanks for catching it.

[mitchell-stellar]

LGTM. Well done!

[HazardyKnusperkeks]

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

clang/unittests/Format/FormatTestComments.cpp

4319

Unrelated?

[owenpan]

clang/unittests/Format/FormatTest.cpp

6371–6380

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;

clang/unittests/Format/FormatTestComments.cpp

4319

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

[owenpan]

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.

[owenpan]

clang/unittests/Format/FormatTest.cpp

6394

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

6371–6380

Fascinating. But wouldn't the better fix be that the directives are not marked as finalized?

clang/unittests/Format/FormatTestComments.cpp

4319

Okay, but that wasn't really trailing, it was part of a string.

[owenpan]

clang/unittests/Format/FormatTest.cpp

6371–6380

I tried that first, but it broke a lot of unit tests. I'll give it another try when I have time. :)

clang/unittests/Format/FormatTestComments.cpp

4319

Yeah. Fixed in e9acf00. Thanks!

[owenpan]

clang/unittests/Format/FormatTest.cpp

6371–6380

See D153243.

clang/lib/Format/WhitespaceManager.cpp

: Tok(&Tok), CreateReplacement(CreateReplacement),
       PreviousEndOfTokenColumn(0), EscapedNewlineColumn(0),
       StartOfBlockComment(nullptr), IndentationOffset(0), ConditionalsLevel(0) {
 }
 
 void WhitespaceManager::replaceWhitespace(FormatToken &Tok, unsigned Newlines,
                                           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, "", "",
                            IsAligned, InPPDirective && !Tok.IsFirst,
                            /*IsInsideToken=*/false));
 }
 
 void WhitespaceManager::addUntouchableToken(const FormatToken &Tok,
static unsigned AlignTokens(const FormatStyle &Style, F &&Matches,
   unsigned EndOfSequence = 0;
 
   // Measure the scope level (i.e. depth of (), [], {}) of the first token, and
   // abort when we hit any token in a higher scope than the starting one.
   auto IndentAndNestingLevel = StartAt < Changes.size()
                                    ? Changes[StartAt].indentAndNestingLevel()
                                    : std::tuple<unsigned, unsigned, unsigned>();
 
-  // 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<unsigned, unsigned, unsigned>();
-
   // 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.
   unsigned CommasBeforeLastMatch = 0;
   unsigned CommasBeforeMatch = 0;
 
   // Whether a matching token has been found on the current line.
   bool FoundMatchOnLine = false;
auto AlignCurrentSequence = [&] {
     WidthAnchor = 0;
     WidthRight = 0;
     StartOfSequence = 0;
     EndOfSequence = 0;
   };
 
   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 finishing this scope block.
-      // Instead, align, and then lower the baseline indent and nesting level
-      // in order to continue aligning subsequent blocks.
-      EndOfSequence = i;
-      AlignCurrentSequence();
-      IndentAndNestingLevel =
-          Changes[i].indentAndNestingLevel(); // new baseline
-    }
+    if (Changes[i].indentAndNestingLevel() < IndentAndNestingLevel)
 
     if (Changes[i].NewlinesBefore != 0) {
       CommasBeforeMatch = 0;
       EndOfSequence = i;
 
       // Whether to break the alignment sequence because of an empty line.
       bool EmptyLineBreak =
           (Changes[i].NewlinesBefore > 1) && !ACS.AcrossEmptyLines;

clang/unittests/Format/FormatTest.cpp

verifyFormat("void f1() {\n"
                "#else\n"
                "  // prevent alignment with #else in f1\n"
                "  char *foobarbaz = \"foobarbaz\";\n"
                "  int   quux      = 4;\n"
                "#endif\n"
                "#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) {
   verifyFormat("{\n  { a #c; }\n}");
 }
 
 TEST_F(FormatTest, FormatUnbalancedStructuralElements) {
   EXPECT_EQ("#define A \\\n  {       \\\n    {\nint i;",

clang/unittests/Format/FormatTestComments.cpp

EXPECT_EQ("int a; // line about a\n"
             format("int a; // line about a\n"
                    "       // line 1 about b\n"
                    "       // line 2 about b\n"
                    "       long b;",
                    getLLVMStyleWithColumns(80)));
 
   // 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"
       "#endif\n"
       "#if BAR\n"
       "#else\n"
       "long b_long_name; // Line about b\n"
       "#endif\n",

 
 TEST_F(FormatTestComments, SplitCommentIntroducers) {
   EXPECT_EQ(R"(//
 /\
 /
 )",
             format(R"(//
 /\
-/ 
+/
   )",
                    getLLVMStyleWithColumns(10)));
 }
 
 } // end namespace
 } // namespace test
 } // end namespace format
 } // end namespace clang