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

[zequanwu]

Summary

Originally filed at https://bugs.chromium.org/p/chromium/issues/detail?id=1184570.
Example:

$ cat a.cpp
#define M(x)
namespace M(ns) {
  void test() {}
}

$ clang-format a.cpp
#define M(x)
namespace M(ns) {
  void test() {}
} // namespace )

When the name of a namespace is a macro that takes arguments,

• It fixed the indentation.
• It fixed the namespace end comments.

[MyDeveloperDay]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

76

but its not an anonymous namespace?

would

'''
}// namespace M(x)
'''

be clearer?

86

same here its not A::M is it? at best its A::x but it depends on the macro ? or have I miss understood?

[zequanwu]

Address comment.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

86

Yes, you are right. Fixed to include the macro literally.
Ideally, it should expand the macro to get the name or ignore the macro.

[curdeius]

Please update the summary, the name is no longer empty. Also, if you can add some info in the summary about how it looked before (copying a part of the Chromium bug report seems enough), it would be awesome.
Otherwise, some small comments only.

clang/lib/Format/NamespaceEndCommentsFixer.cpp

59

60

68

It should be simpler when written as !Tok->isOneOf(...).

[MyDeveloperDay]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

112

Is this 2 bugs in one? I notice you also handling attributes? is this a different bug? (if so it should really be separate (but we can let it slide as long as the tests are thorough)

can you test:

namespace /* comment  */ [[ xxx ]]  /* comment */  A {
int i;
int j;
}  // namespace  A

namespace /* comment  */ [[ xxx ]]   A {
int i;
int j;
}  //  namespace A

namespace /* comment  */ [[ xxx ]]   /* comment */ M(x) {
int i;
int j;
}  // namespace  M(x)

namespace /* comment  */ [[ xxx ]]   /* comment */  A::M(x) {
int i;
int j;
}  // namespace  A::M(x)

namespace /* comment  */ [[ xxx ]]   /* comment */ M(x)  /* comment */ {
int i;
int j;
}  // namespace  M(x)  

namespace /* comment  */ [[ xxx ]]   /* comment */  A::M(x) /* comment */  {
int i;
int j;
}  // namespace  A::M(x)

[MyDeveloperDay]

Its assignment only happens in MacroExpander which is never used

This is something that @klimek was working on.

[zequanwu]

Address comments.

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

112

Is this 2 bugs in one? I notice you also handling attributes?

No. This tests with attribute is here to test that candidate name doesn't include attributes, but that is unnecessary. Added the 6 tests above for testing that.

[MyDeveloperDay]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

112

Where are you checking the addition of this

if (Tok && Tok->is(tok::l_square)) {
      int NestLevel = 1;
      while (Tok && NestLevel > 0) {
        Tok = Tok->getNextNonComment();
        if (Tok) {
          if (Tok->is(tok::l_square))
            ++NestLevel;
          if (Tok->is(tok::r_square))
            --NestLevel;
        }
      }
      if (Tok)
        Tok = Tok->getNextNonComment();
    }

[zequanwu]

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

112

That part of code skips attribute so that FirstNSName doesn't add attribute string into its name.
In the following test case. it's tested.

namespace /* comment  */ [[ xxx ]]   /* comment */  A::M(x) {
int i;
int j;
}  // namespace  A::M(x)

[owenpan]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

45–54

To make the loop simpler and more efficient.

59–60

Move "as a name candidate" to between "namespace" and "until".

[owenpan]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

59–60

Move "as a name candidate" to between "namespace" and "until".

Or move it to after "Use", i.e. "Use as a name candidate the string after ...".

[zequanwu]

update.

[owenpan]

LGTM, but please wait for other reviewers in case they have more comments.

clang/lib/Format/NamespaceEndCommentsFixer.cpp

if (NamespaceTok->is(TT_NamespaceMacro)) {
     // 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();
+    }
+
+    // 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 '{') and then return all tokens from one
-    // token before that up until the '{'.
+    // 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->is(tok::l_brace) && !Tok->is(tok::coloncolon)) {
+    while (Tok && !Tok->isOneOf(tok::l_brace, tok::coloncolon, tok::l_paren)) {
+      FirstNSName += FirstNSTok->TokenText;
       FirstNSTok = Tok;
       Tok = Tok->getNextNonComment();
     }
 
     Tok = FirstNSTok;
     while (Tok && !Tok->is(tok::l_brace)) {
       name += Tok->TokenText;
       if (Tok->is(tok::kw_inline))
         name += " ";
       Tok = Tok->getNextNonComment();
     }
+    if (name.empty())
+      name = FirstNSName;
   }
   return name;
 }
 
 std::string computeEndCommentText(StringRef NamespaceName, bool AddNewline,
                                   const FormatToken *NamespaceTok,
                                   unsigned SpacesToAdd) {
   std::string text = "//";

clang/lib/Format/UnwrappedLineParser.cpp

assert(FormatTok->isOneOf(tok::kw_namespace, TT_NamespaceMacro) &&
          "'namespace' expected");
 
   const FormatToken &InitialToken = *FormatTok;
   nextToken();
   if (InitialToken.is(TT_NamespaceMacro)) {
     parseParens();
   } else {
     while (FormatTok->isOneOf(tok::identifier, tok::coloncolon, tok::kw_inline,
-                              tok::l_square, tok::period) ||
+                              tok::l_square, tok::period, tok::l_paren) ||
            (Style.isCSharp() && FormatTok->is(tok::kw_union)))
       if (FormatTok->is(tok::l_square))
         parseSquare();
+      else if (FormatTok->is(tok::l_paren))
+        parseParens();
       else
         nextToken();
   }
   if (FormatTok->is(tok::l_brace)) {
     if (ShouldBreakBeforeBrace(Style, InitialToken))
       addUnwrappedLine();
 
     unsigned AddLevels =

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, FormatsNamespaces) {
   FormatStyle LLVMWithNoNamespaceFix = getLLVMStyle();
   LLVMWithNoNamespaceFix.FixNamespaceComments = false;
 
   verifyFormat("namespace some_namespace {\n"
                "class A {};\n"
                "void f() { f(); }\n"
                "}",
                LLVMWithNoNamespaceFix);
+  verifyFormat("#define M(x) x##x\n"
+               "namespace M(x) {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
+  verifyFormat("#define M(x) x##x\n"
+               "namespace N::inline M(x) {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
+  verifyFormat("#define M(x) x##x\n"
+               "namespace M(x)::inline N {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
+  verifyFormat("#define M(x) x##x\n"
+               "namespace N::M(x) {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
+  verifyFormat("#define M(x) x##x\n"
+               "namespace M::N(x) {\n"
+               "class A {};\n"
+               "void f() { f(); }\n"
+               "}",
+               LLVMWithNoNamespaceFix);
   verifyFormat("namespace N::inline D {\n"
                "class A {};\n"
                "void f() { f(); }\n"
                "}",
                LLVMWithNoNamespaceFix);
   verifyFormat("namespace N::inline D::E {\n"
                "class A {};\n"
                "void f() { f(); }\n"

clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp

TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
   EXPECT_EQ("namespace A {\n"
             "int i;\n"
             "int j;\n"
             "}// namespace A",
             fixNamespaceEndComments("namespace A {\n"
                                     "int i;\n"
                                     "int j;\n"
                                     "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace M(x) {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace M(x)",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace M(x) {\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 M(x) x##x\n"
+            "namespace M(x)::A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace M(x)::A",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace M(x)::A {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace A::inline M(x)::B {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A::inline M(x)::B",
+            fixNamespaceEndComments("#define M(x) x##x\n"
+                                    "namespace A::inline M(x)::B {\n"
+                                    "int i;\n"
+                                    "int j;\n"
+                                    "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace [[deprecated(\"foo\")]] A::inline M(x)::A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A::inline M(x)::A",
+            fixNamespaceEndComments(
+                "#define M(x) x##x\n"
+                "namespace [[deprecated(\"foo\")]] A::inline M(x)::A {\n"
+                "int i;\n"
+                "int j;\n"
+                "}"));
+  EXPECT_EQ(
+      "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ A {\n"
+      "int i;\n"
+      "int j;\n"
+      "}// namespace A",
+      fixNamespaceEndComments(
+          "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ A {\n"
+          "int i;\n"
+          "int j;\n"
+          "}"));
+  EXPECT_EQ("namespace /* comment */ [[deprecated(\"foo\")]] A {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A",
+            fixNamespaceEndComments(
+                "namespace /* comment */ [[deprecated(\"foo\")]] A {\n"
+                "int i;\n"
+                "int j;\n"
+                "}"));
+  EXPECT_EQ(
+      "#define M(x) x##x\n"
+      "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ M(x) {\n"
+      "int i;\n"
+      "int j;\n"
+      "}// namespace M(x)",
+      fixNamespaceEndComments("#define M(x) x##x\n"
+                              "namespace /* comment */ "
+                              "[[deprecated(\"foo\")]] /* comment */ M(x) {\n"
+                              "int i;\n"
+                              "int j;\n"
+                              "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ "
+            "A::M(x) {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace A::M(x)",
+            fixNamespaceEndComments(
+                "#define M(x) x##x\n"
+                "namespace /* comment */ "
+                "[[deprecated(\"foo\")]] /* comment */ A::M(x) {\n"
+                "int i;\n"
+                "int j;\n"
+                "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ "
+            "M(x) /* comment */ {\n"
+            "int i;\n"
+            "int j;\n"
+            "}// namespace M(x)",
+            fixNamespaceEndComments(
+                "#define M(x) x##x\n"
+                "namespace /* comment */ [[deprecated(\"foo\")]] /* comment "
+                "*/ M(x) /* comment */ {\n"
+                "int i;\n"
+                "int j;\n"
+                "}"));
+  EXPECT_EQ("#define M(x) x##x\n"
+            "namespace /* comment */ [[deprecated(\"foo\")]] /* comment */ "
+            "A::M(x) /* comment */ {\n"
+            "int i;\n"
+            "int j;\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("inline namespace A {\n"
             "int i;\n"
             "int j;\n"
             "}// namespace A",
             fixNamespaceEndComments("inline namespace A {\n"
                                     "int i;\n"
                                     "int j;\n"
                                     "}"));