[clang-format] [PR45614] Incorrectly indents [[nodiscard]] attribute funtions after a macro without semicolon

[MyDeveloperDay]

Summary

https://bugs.llvm.org/show_bug.cgi?id=45614

[[nodiscard]] after a macro doesn't behave the same as an attribute resulting in incorrect indentation

This revision corrects that behavior

MACRO

__attribute__((warn_unused_result)) int f3(); // ok

MACRO

    [[nodiscard]] int
    f4(); // bad: unexpectedly indented!

See original Mozilla bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1629756

Before:

class FooWidget : public nsBaseWidget {
public:
  FooWidget();

  NS_DECL_ISUPPORTS_INHERITED

      [[nodiscard]] nsresult
      FunctionOne();
  [[nodiscard]] nsresult FunctionTwo();
};

After:

class FooWidget : public nsBaseWidget {
public:
  FooWidget();

  NS_DECL_ISUPPORTS_INHERITED

  [[nodiscard]] nsresult FunctionOne();
  [[nodiscard]] nsresult FunctionTwo();
};

[Abpostelnicu]

@MyDeveloperDay thanks for the patch, I'm gonna run it agains mozilla to see if there is any fallout.

[MyDeveloperDay]

In D79990#2038207, @Abpostelnicu wrote:

@MyDeveloperDay thanks for the patch, I'm gonna run it agains mozilla to see if there is any fallout.

I wonder is there a documented way to run clang-format over the gekco sources? when I make a change to clang-format I like to run it over some real code not just the tests, as I know Mozilla is supposed to be completed formatted this might prove to be a better test than running it over LLVM itself (which is alas not 100% formatted)

[krasimir]

clang/lib/Format/UnwrappedLineParser.cpp

906

nit: pass Tok by const reference (makes sure we don't have to deal with nullptr and as bonus will make this patch diff smaller).

[MyDeveloperDay]

Change to const &

[Abpostelnicu]

Please see https://firefox-source-docs.mozilla.org/code-quality/coding-style/format_cpp_code_with_clang-format.html

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parsePPUnknown() {
   if (Style.IndentPPDirectives != FormatStyle::PPDIS_None)
     Line->Level += PPBranchLevel + 1;
   addUnwrappedLine();
 }
 
 // Here we blacklist certain tokens that are not usually the first token in an
 // unwrapped line. This is used in attempt to distinguish macro calls without
 // trailing semicolons from other constructs split to several lines.
-static bool tokenCanStartNewLine(const clang::Token &Tok) {
+static bool tokenCanStartNewLine(const FormatToken &Tok) {
   // Semicolon can be a null-statement, l_square can be a start of a macro or
   // a C++11 attribute, but this doesn't seem to be common.
   return Tok.isNot(tok::semi) && Tok.isNot(tok::l_brace) &&
-         Tok.isNot(tok::l_square) &&
+         Tok.isNot(TT_AttributeSquare) &&
          // Tokens that can only be used as binary operators and a part of
          // overloaded operator names.
          Tok.isNot(tok::period) && Tok.isNot(tok::periodstar) &&
          Tok.isNot(tok::arrow) && Tok.isNot(tok::arrowstar) &&
          Tok.isNot(tok::less) && Tok.isNot(tok::greater) &&
          Tok.isNot(tok::slash) && Tok.isNot(tok::percent) &&
          Tok.isNot(tok::lessless) && Tok.isNot(tok::greatergreater) &&
          Tok.isNot(tok::equal) && Tok.isNot(tok::plusequal) &&
case tok::identifier: {
           parseParens();
 
         bool FollowedByNewline =
             CommentsBeforeNextToken.empty()
                 ? FormatTok->NewlinesBefore > 0
                 : CommentsBeforeNextToken.front()->NewlinesBefore > 0;
 
         if (FollowedByNewline && (Text.size() >= 5 || FunctionLike) &&
-            tokenCanStartNewLine(FormatTok->Tok) && Text == Text.upper()) {
+            tokenCanStartNewLine(*FormatTok) && Text == Text.upper()) {
           addUnwrappedLine();
           return;
         }
       }
       break;
     }
     case tok::equal:
       // Fat arrows (=>) have tok::TokenKind tok::equal but TokenType

clang/unittests/Format/FormatTest.cpp

verifyFormat("struct S {\n"
                "};",
                Style);
   verifyFormat("struct [[nodiscard]] S {\n"
                "  S(S&&) = default;\n"
                "};",
                Style);
 }
 
+TEST_F(FormatTest, AttributesAfterMacro) {
+  FormatStyle Style = getLLVMStyle();
+  verifyFormat("MACRO;\n"
+               "__attribute__((maybe_unused)) int foo() {\n"
+               "  //...\n"
+               "}");
+
+  verifyFormat("MACRO;\n"
+               "[[nodiscard]] int foo() {\n"
+               "  //...\n"
+               "}");
+
+  EXPECT_EQ("MACRO\n\n"
+            "__attribute__((maybe_unused)) int foo() {\n"
+            "  //...\n"
+            "}",
+            format("MACRO\n\n"
+                   "__attribute__((maybe_unused)) int foo() {\n"
+                   "  //...\n"
+                   "}"));
+
+  EXPECT_EQ("MACRO\n\n"
+            "[[nodiscard]] int foo() {\n"
+            "  //...\n"
+            "}",
+            format("MACRO\n\n"
+                   "[[nodiscard]] int foo() {\n"
+                   "  //...\n"
+                   "}"));
+}
+
 TEST_F(FormatTest, AttributePenaltyBreaking) {
   FormatStyle Style = getLLVMStyle();
   verifyFormat("void ABCDEFGH::ABCDEFGHIJKLMN(\n"
                "    [[maybe_unused]] const shared_ptr<ALongTypeName> &C d) {}",
                Style);
   verifyFormat("void ABCDEFGH::ABCDEFGHIJK(\n"
                "    [[maybe_unused]] const shared_ptr<ALongTypeName> &C d) {}",
                Style);