[clang-format] Fix FixNamespaceComments when BraceWrapping AfterNamespace is true.

[curdeius]

Summary

NamespaceEndCommentsFixer did not fix namespace comments when the brace opening the namespace was not on the same line as the "namespace" keyword.
It occurs in Allman, GNU and Linux styles and whenever BraceWrapping.AfterNamespace is true.

Before:

namespace a
{
void f();
void g();
}

After:

namespace a
{
void f();
void g();
} // namespace a

[krasimir]

Could you please move the test that adds a namespace end comment to unittests/Format/NamespaceEndCommentsFixerTest.cpp?

[curdeius]

In D37904#873860, @krasimir wrote:

Could you please move the test that adds a namespace end comment to unittests/Format/NamespaceEndCommentsFixerTest.cpp?

That's what I wanted to do initially, but the very same tests there passed surprisingly. I think that there are some differences related to "messing up" the code between both test suites.
I'll have another look at it however.

[curdeius]

I confirm what I observed before: when invoking tests in unittests/Format/NamespaceEndCommentsFixerTest.cpp, the const AnnotatedLine *line parameter in getNamespaceToken gets one big line that includes both namespace and { (something like namespace\n{\n..., whereas in tests invoked from unittests/Format/FormatTests.cpp, there is a separate line with namespace\n and another one with {\n.

I haven't looked at what provokes this behavior, but I imagine that there are additional passes in FormatTests.

[krasimir]

This is how you could add a test in NamespaceEndCommentsFixerTest.cpp:

TEST_F(NamespaceEndCommentsFixerTest, FixesNamespaceCommentsInAllmanStyle) {
  FormatStyle AllmanStyle = getLLVMStyle();
  AllmanStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
  EXPECT_EQ("namespace a\n"
            "{\n"
            "void f();\n"
            "void g();\n"
            "}// namespace a\n",
            fixNamespaceEndComments("namespace a\n"
                                    "{\n"
                                    "void f();\n"
                                    "void g();\n"
                                    "}\n",
                                    AllmanStyle));
}

[curdeius]

That's precisely what I've written, but, as I'd said before, such tests pass already without any modification in NamespaceEndCommentsFixer.

[curdeius]

Any thoughts on this?

[krasimir]

Looks good!

I must have gotten confused with my test suggestion.
Do you have commit access, or should I commit this for you?

lib/Format/NamespaceEndCommentsFixer.cpp

     return nullptr;
   assert(StartLineIndex < AnnotatedLines.size());
   const FormatToken *NamespaceTok = AnnotatedLines[StartLineIndex]->First;
+  if (NamespaceTok->is(tok::l_brace)) {
+    // "namespace" keyword can be on the line preceding '{', e.g. in styles
+    // where BraceWrapping.AfterNamespace is true.
+    if (StartLineIndex > 0)
+      NamespaceTok = AnnotatedLines[StartLineIndex - 1]->First;
+  }
   // Detect "(inline)? namespace" in the beginning of a line.
   if (NamespaceTok->is(tok::kw_inline))
     NamespaceTok = NamespaceTok->getNextNonComment();

unittests/Format/FormatTest.cpp

   verifyFormat("typedef enum {} EmptyEnum;");
   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum {\n"
-               "  ZERO = 0,\n"             
+               "  ZERO = 0,\n"
                "  ONE = 1,\n"
                "  TWO = 2,\n"
                "  THREE = 3\n"

   verifyFormat("typedef enum { A, B, C } ShortEnum;");
   verifyFormat("typedef enum\n"
                "{\n"
-               "  ZERO = 0,\n"             
+               "  ZERO = 0,\n"
                "  ONE = 1,\n"
                "  TWO = 2,\n"
                "  THREE = 3\n"

                "struct B {\n"
                "  int x;\n"
                "};\n"
-               "}\n",
+               "} // namespace a\n",
                LinuxBraceStyle);
   verifyFormat("enum X {\n"
                "  Y = 0,\n"

 TEST_F(FormatTest, AllmanBraceBreaking) {
   FormatStyle AllmanBraceStyle = getLLVMStyle();
   AllmanBraceStyle.BreakBeforeBraces = FormatStyle::BS_Allman;
+
+  EXPECT_EQ("namespace a\n"
+            "{\n"
+            "void f();\n"
+            "void g();\n"
+            "} // namespace a\n",
+            format("namespace a\n"
+                   "{\n"
+                   "void f();\n"
+                   "void g();\n"
+                   "}\n",
+                   AllmanBraceStyle));
+
   verifyFormat("namespace a\n"
                "{\n"
                "class A\n"

                "{\n"
                "  int x;\n"
                "};\n"
-               "}",
+               "} // namespace a",
                AllmanBraceStyle);
 
   verifyFormat("void f()\n"

                "  }\n"
                "  void g() { return; }\n"
                "}\n"
-               "}",
+               "} // namespace a",
                GNUBraceStyle);
 
   verifyFormat("void f()\n"