[clang-format] Fix formatting of macro definitions with a leading comment.

[curdeius]

Summary

Fixes https://github.com/llvm/llvm-project/issues/43206.

[curdeius]

Add a test case. Use bool instead of Previous token.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

3574

I really don't like bitwise operations on bool. Hit some nasty bugs at my job.

3589

This does not change within the loop, is that on purpose? Then it should be an if around the loop.

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

3574

Ok, will do.

3589

Oh, good catch. It was certainly not on purpose. And it means that I need to find a new test case that covers that (if appropriate). Ideas welcome!

[curdeius]

I'll test it tomorrow but probably a define with a hash as the last character may do the job and show the faulty behaviour.

[curdeius]

Indeed, I get the hash on the 2nd line merged into the first one in this test case:

verifyFormat("/* comment */ #define A (parentheses)\n"
            "#");

But! It's unrelated to whether PreviousWasComment is updated inside the loop or not.
Actually it should not be important because # always starts a preprocessor macro and no other statement (at least in C and C++).
On the other hand, FirstNonCommentOnLine should be updated in the loop but I still don't have a test case for this (again, because there's no statement that starts with #).

Interestingly, the problem I encountered with the test case above doesn't happen when there's a new line after #:

verifyFormat("/* comment */ #define A (parentheses)\n"
             "#\n");

Anyway, this merging happens due to something else, but I don't know yet what exactly.
Investigating...

[curdeius]

Add tests, update in loop.

[curdeius]

Don't use |=.

[HazardyKnusperkeks]

clang/lib/Format/UnwrappedLineParser.cpp

3612

There it is again. ;)

The if is the same as above, isn't it?

[curdeius]

Clean up.

[curdeius]

@HazardyKnusperkeks, sorry for that. Should be better now.

[HazardyKnusperkeks]

In D118924#3300981, @curdeius wrote:

@HazardyKnusperkeks, sorry for that. Should be better now.

No problem here.

clang/lib/Format/UnwrappedLineParser.cpp

if (ShouldPushCommentsInCurrentLine)
       pushToken(FormatTok);
     else
       CommentsBeforeNextToken.push_back(FormatTok);
   }
 }
 
 void UnwrappedLineParser::readToken(int LevelDifference) {
   SmallVector<FormatToken *, 1> Comments;
+  bool PreviousWasComment = false;
+  bool FirstNonCommentOnLine = false;
   do {
     FormatTok = Tokens->getNextToken();
     assert(FormatTok);
     while (FormatTok->getType() == TT_ConflictStart ||
            FormatTok->getType() == TT_ConflictEnd ||
            FormatTok->getType() == TT_ConflictAlternative) {
       if (FormatTok->getType() == TT_ConflictStart)
         conditionalCompilationStart(/*Unreachable=*/false);
       else if (FormatTok->getType() == TT_ConflictAlternative)
         conditionalCompilationAlternative();
       else if (FormatTok->getType() == TT_ConflictEnd)
         conditionalCompilationEnd();
       FormatTok = Tokens->getNextToken();
       FormatTok->MustBreakBefore = true;
     }
 
+    auto IsFirstNonCommentOnLine = [](bool FirstNonCommentOnLine,
+                                      const FormatToken &Tok,
+                                      bool PreviousWasComment) {
+      auto IsFirstOnLine = [](const FormatToken &Tok) {
+        return Tok.HasUnescapedNewline || Tok.IsFirst;
+      };
+
+      // Consider preprocessor directives preceded by block comments as first
+      // on line.
+      if (PreviousWasComment)
+        return FirstNonCommentOnLine || IsFirstOnLine(Tok);
+      return IsFirstOnLine(Tok);
+    };
+
+    FirstNonCommentOnLine = IsFirstNonCommentOnLine(
+        FirstNonCommentOnLine, *FormatTok, PreviousWasComment);
+    PreviousWasComment = FormatTok->Tok.is(tok::comment);
+
     while (!Line->InPPDirective && FormatTok->Tok.is(tok::hash) &&
-           (FormatTok->HasUnescapedNewline || FormatTok->IsFirst)) {
+           FirstNonCommentOnLine) {
       distributeComments(Comments, FormatTok);
       Comments.clear();
       // If there is an unfinished unwrapped line, we flush the preprocessor
       // directives only after that unwrapped line was finished later.
       bool SwitchToPreprocessorLines = !Line->Tokens.empty();
       ScopedLineState BlockState(*this, SwitchToPreprocessorLines);
       assert((LevelDifference >= 0 ||
               static_cast<unsigned>(-LevelDifference) <= Line->Level) &&
              "LevelDifference makes Line->Level negative");
       Line->Level += LevelDifference;
       // Comments stored before the preprocessor directive need to be output
       // before the preprocessor directive, at the same level as the
       // preprocessor directive, as we consider them to apply to the directive.
       if (Style.IndentPPDirectives == FormatStyle::PPDIS_BeforeHash &&
           PPBranchLevel > 0)
         Line->Level += PPBranchLevel;
       flushComments(isOnNewLine(*FormatTok));
       parsePPDirective();
+      PreviousWasComment = FormatTok->Tok.is(tok::comment);
+      FirstNonCommentOnLine = IsFirstNonCommentOnLine(
+          FirstNonCommentOnLine, *FormatTok, PreviousWasComment);
     }
 
     if (!PPStack.empty() && (PPStack.back().Kind == PP_Unreachable) &&
         !Line->InPPDirective)
       continue;
 
     if (!FormatTok->Tok.is(tok::comment)) {
       distributeComments(Comments, FormatTok);
       Comments.clear();
       return;

clang/unittests/Format/FormatTest.cpp

verifyFormat("BOOST_FOREACH (int v, vec)\n"
                "{\n"
                "  f();\n"
                "}",
                AllowSimpleBracedStatements);
 }
 
 TEST_F(FormatTest, UnderstandsMacros) {
   verifyFormat("#define A (parentheses)");
+  verifyFormat("/* comment */ #define A (parentheses)");
+  verifyFormat("/* comment */ /* another comment */ #define A (parentheses)");
+  // Even the partial code should never be merged.
+  EXPECT_EQ("/* comment */ #define A (parentheses)\n"
+            "#",
+            format("/* comment */ #define A (parentheses)\n"
+                   "#"));
+  verifyFormat("/* comment */ #define A (parentheses)\n"
+               "#\n");
+  verifyFormat("/* comment */ #define A (parentheses)\n"
+               "#define B (parentheses)");
   verifyFormat("#define true ((int)1)");
   verifyFormat("#define and(x)");
   verifyFormat("#define if(x) x");
   verifyFormat("#define return(x) (x)");
   verifyFormat("#define while(x) for (; x;)");
   verifyFormat("#define xor(x) (^(x))");
   verifyFormat("#define __except(x)");
   verifyFormat("#define __try(x)");