[clang-format] Fix namespace format when the name is followed by a macro

[zequanwu]

Summary

Example:

$ cat a.cpp
namespace my_namespace::yeah API_AVAILABLE(macos(10.15)) {
void test() {}
}

$ clang-format a.cpp
namespace my_namespace::yeah API_AVAILABLE(macos(10.15)) {
void test() {}
}// namespace my_namespace::yeahAPI_AVAILABLE(macos(10.15))

After:

$ clang-format a.cpp
namespace my_namespace::yeah API_AVAILABLE(macos(10.15)) {
void test() {}
}// namespace my_namespace::yeah

[MyDeveloperDay]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

145

Elide braces

[MyDeveloperDay]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

145

Ok ignore that

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

192

Can you test the A B case? We can’t have a space right?

[MyDeveloperDay]

Please wait for the other reviewers

[thakis]

(deferring to MyDeveloperDay and curdeius)

[curdeius]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

192–211

What's the rationale behind keeping M(x) in one case and not the other?
How can you decide?

[zequanwu]

Add a test for namespace A B {.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

192

Added a test with A B.
In that case, we will have // namespace A B, since we don't know which one is the real name. Either one could be a macro that expands to an attribute.

192–211

In the first one, we keep M(x) because we don't know that is A or M(x) the name.
In the second one, because we see a ::, so we know that's the name and discard M(x).

[curdeius]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

96–97

Nit: Personally, I'd put these bools further down, closer to the place where they are used and after AddMacro lambda.

126

This loop seems very similar to the one that skips the attributes on lines 45-53, could you please refactor?
A lambda taking a pair of token kinds (l_paren/r_paren, l_square/r_square) should be fine.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

192

In a different test case, could you add A to AttributeMacros and test that we skip it like other attributes?

192

Euh, actually, it's out of scope of this patch. Please ignore.

[zequanwu]

Refactor.
Add a test for namespace A __attribute__((availability(macos, introduced=10.15))) {.

[curdeius]

LGTM % nits. Thanks for working on this!

clang/lib/Format/NamespaceEndCommentsFixer.cpp

54–55

Nit: add braces around else if block to match the if.

76–77

Comment not necessary anymore. Please remove.

127

Comment not necessary anymore. Please remove.

132

[owenpan]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

33

And other places below where applicable.

76–77

Early return.

131–134

Something like that.

139

[zequanwu]

Address comments.

[owenpan]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

108

116

135–139

clang/lib/Format/NamespaceEndCommentsFixer.cpp

 //===--- NamespaceEndCommentsFixer.cpp --------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file implements NamespaceEndCommentsFixer, a TokenAnalyzer that
 /// fixes namespace end comments.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "NamespaceEndCommentsFixer.h"
+#include "clang/Basic/TokenKinds.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Regex.h"
 
 #define DEBUG_TYPE "namespace-end-comments-fixer"
 
 namespace clang {
 namespace format {
 
 namespace {
+// Iterates all tokens starting from StartTok to EndTok and apply Fn to all
+// tokens between them including StartTok and EndTok. Returns the token after
+// EndTok.
+const FormatToken *
+processTokens(const FormatToken *Tok, tok::TokenKind StartTok,
+              tok::TokenKind EndTok,
+              llvm::function_ref<void(const FormatToken *)> Fn) {
+  if (!Tok || Tok->isNot(StartTok))
+    return Tok;
+  int NestLevel = 0;
+  do {
+    if (Tok->is(StartTok))
+      ++NestLevel;
+    else if (Tok->is(EndTok))
+      --NestLevel;
+    if (Fn)
+      Fn(Tok);
+    Tok = Tok->getNextNonComment();
+  } while (Tok && NestLevel > 0);
+  return Tok;
+}
+
+const FormatToken *skipAttribute(const FormatToken *Tok) {
+  if (!Tok)
+    return nullptr;
+  if (Tok->is(tok::kw___attribute)) {
+    Tok = Tok->getNextNonComment();
+    Tok = processTokens(Tok, tok::l_paren, tok::r_paren, nullptr);
+  } else if (Tok->is(tok::l_square)) {
+    Tok = processTokens(Tok, tok::l_square, tok::r_square, nullptr);
+  }
+  return Tok;
+}
+
 // Computes the name of a namespace given the namespace token.
 // Returns "" for anonymous namespace.
 std::string computeName(const FormatToken *NamespaceTok) {
   assert(NamespaceTok &&
          NamespaceTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
          "expecting a namespace token");
   std::string name;
   const FormatToken *Tok = NamespaceTok->getNextNonComment();
   if (NamespaceTok->is(TT_NamespaceMacro)) {
     // Collects all the non-comment tokens between opening parenthesis
     // and closing parenthesis or comma.
     assert(Tok && Tok->is(tok::l_paren) && "expected an opening parenthesis");
     Tok = Tok->getNextNonComment();
     while (Tok && !Tok->isOneOf(tok::r_paren, tok::comma)) {
       name += Tok->TokenText;
       Tok = Tok->getNextNonComment();
     }
-  } else {
-    // Skip attributes.
-    if (Tok && Tok->is(tok::l_square)) {
-      for (int NestLevel = 1; NestLevel > 0;) {
-        Tok = Tok->getNextNonComment();
-        if (!Tok)
-          break;
-        if (Tok->is(tok::l_square))
-          ++NestLevel;
-        else if (Tok->is(tok::r_square))
-          --NestLevel;
-      }
-      if (Tok)
-        Tok = Tok->getNextNonComment();
-    }
+    return name;
+  Tok = skipAttribute(Tok);
 
-    // Use the string after `namespace` as a name candidate until `{` or `::` or
-    // `(`. If the name is empty, use the candicate.
-    std::string FirstNSName;
-    // For `namespace [[foo]] A::B::inline C {` or
-    // `namespace MACRO1 MACRO2 A::B::inline C {`, returns "A::B::inline C".
-    // Peek for the first '::' (or '{' or '(')) and then return all tokens from
-    // one token before that up until the '{'. A '(' might be a macro with
-    // arguments.
-    const FormatToken *FirstNSTok = Tok;
-    while (Tok && !Tok->isOneOf(tok::l_brace, tok::coloncolon, tok::l_paren)) {
+  const FormatToken *FirstNSTok = nullptr;
+    if (FirstNSTok)
       FirstNSName += FirstNSTok->TokenText;
-      FirstNSTok = Tok;
-      Tok = Tok->getNextNonComment();
-    }
 
+  if (FirstNSTok)
     Tok = FirstNSTok;
-    while (Tok && !Tok->is(tok::l_brace)) {
-      name += Tok->TokenText;
-      if (Tok->is(tok::kw_inline))
+  Tok = skipAttribute(Tok);
+
+  FirstNSTok = nullptr;
+  // Add everything from '(' to ')'.
+  auto AddToken = [&name](const FormatToken *Tok) { name += Tok->TokenText; };
+  bool IsPrevColoncolon = false;
+  bool HasColoncolon = false;
+  bool IsPrevInline = false;
+  bool NameFinished = false;
+  // If we found '::' in name, then it's the name. Otherwise, we can't tell
+  // which one is name. For example, `namespace A B {`.
+  while (Tok && Tok->isNot(tok::l_brace)) {
+    if (FirstNSTok) {
+      if (!IsPrevInline && HasColoncolon && !IsPrevColoncolon) {
+        if (FirstNSTok->is(tok::l_paren)) {
+          FirstNSTok = Tok =
+              processTokens(FirstNSTok, tok::l_paren, tok::r_paren, AddToken);
+          continue;
+        }
+        if (FirstNSTok->isNot(tok::coloncolon)) {
+          NameFinished = true;
+          break;
+        }
+      }
+      name += FirstNSTok->TokenText;
+      IsPrevColoncolon = FirstNSTok->is(tok::coloncolon);
+      HasColoncolon = HasColoncolon || IsPrevColoncolon;
+      if (FirstNSTok->is(tok::kw_inline)) {
         name += " ";
-      Tok = Tok->getNextNonComment();
+        IsPrevInline = true;
       }
-    if (name.empty())
-      name = FirstNSName;
     }
+    FirstNSTok = Tok;
+    Tok = Tok->getNextNonComment();
+    const FormatToken *TokAfterAttr = skipAttribute(Tok);
+    if (TokAfterAttr != Tok)
+      FirstNSTok = Tok = TokAfterAttr;
+  }
+  if (!NameFinished && FirstNSTok && FirstNSTok->isNot(tok::l_brace))
+    name += FirstNSTok->TokenText;
+  if (FirstNSName.empty() || HasColoncolon)
     return name;
+  return name.empty() ? FirstNSName : FirstNSName + " " + name;
 }
 
 std::string computeEndCommentText(StringRef NamespaceName, bool AddNewline,
                                   const FormatToken *NamespaceTok,
                                   unsigned SpacesToAdd) {
   std::string text = "//";
   text.append(SpacesToAdd, ' ');
   text += NamespaceTok->TokenText;
   if (NamespaceTok->is(TT_NamespaceMacro))
     text += "(";
   else if (!NamespaceName.empty())
     text += ' ';
   text += NamespaceName;
   if (NamespaceTok->is(TT_NamespaceMacro))

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

EXPECT_EQ("#define M(x) x##x\n"
             "}// namespace A::M(x)",
             fixNamespaceEndComments(
                 "#define M(x) x##x\n"
                 "namespace /* comment */ [[deprecated(\"foo\")]] /* comment "
                 "*/ A::M(x) /* comment */ {\n"
                 "int i;\n"
                 "int j;\n"
                 "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A M(x) {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A M(x)",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace A M(x) {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ(
+      "#define B __attribute__((availability(macos, introduced=10.15)))\n"
+      "namespace A B {\n"
+      "int i;\n"
+      "int j;\n"
+      "}// namespace A B",
+      fixNamespaceEndComments(
+          "#define B __attribute__((availability(macos, introduced=10.15)))\n"
+          "namespace A B {\n"
+          "int i;\n"
+          "int j;\n"
+          "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A::B M(x) {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A::B",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace A::B M(x) {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ(
+      "namespace A __attribute__((availability(macos, introduced=10.15))) {\n"
+      "int i;\n"
+      "int j;\n"
+      "}// namespace A",
+      fixNamespaceEndComments(
+          "namespace A __attribute__((availability(macos, introduced=10.15))) "
+          "{\n"
+          "int i;\n"
+          "int j;\n"
+          "}"));
   EXPECT_EQ("inline namespace A {\n"
             "int i;\n"
             "int j;\n"
             "}// namespace A",
             fixNamespaceEndComments("inline namespace A {\n"
                                     "int i;\n"
                                     "int j;\n"
                                     "}"));