[clang-format] Use number of unwrapped lines for short namespace

[krasimir]

Summary

This patch makes the namespace comment fixer use the number of unwrapped lines
that a namespace spans to detect it that namespace is short, thus not needing
end comments to be added.
This is needed to ensure clang-format is idempotent. Previously, a short namespace
was detected by the original source code lines. This has the effect of requiring two
runs for this example:

namespace { class A; }

after first run:

namespace {
class A;
}

after second run:

namespace {
class A;
} // namespace

[djasper]

Please include the reasoning in the patch description, i.e. that otherwise clang-format might need to runs to add all the namespace comments.

lib/Format/NamespaceEndCommentsFixer.cpp

 #include "llvm/Support/Regex.h"
 
 #define DEBUG_TYPE "namespace-end-comments-fixer"
 
 namespace clang {
 namespace format {
 
 namespace {
-// The maximal number of lines that a short namespace spans.
+// The maximal number of unwrapped lines that a short namespace spans.
 // Short namespaces don't need an end comment.
 static const int kShortNamespaceMaxLines = 1;
 
 // Matches a valid namespace end comment.
 // Valid namespace end comments don't need to be edited.
 static llvm::Regex kNamespaceCommentPattern =
     llvm::Regex("^/[/*] *(end (of )?)? *(anonymous|unnamed)? *"
                 "namespace( +([a-zA-Z0-9:_]+))?\\.? *(\\*/)?$",
if (!NamespaceName.empty()) {
     text += ' ';
     text += NamespaceName;
   }
   if (AddNewline)
     text += '\n';
   return text;
 }
 
-bool isShort(const FormatToken *NamespaceTok, const FormatToken *RBraceTok,
-             const SourceManager &SourceMgr) {
-  int StartLine =
-      SourceMgr.getSpellingLineNumber(NamespaceTok->Tok.getLocation());
-  int EndLine = SourceMgr.getSpellingLineNumber(RBraceTok->Tok.getLocation());
-  return EndLine - StartLine + 1 <= kShortNamespaceMaxLines;
-}
-
 bool hasEndComment(const FormatToken *RBraceTok) {
   return RBraceTok->Next && RBraceTok->Next->is(tok::comment);
 }
 
 bool validEndComment(const FormatToken *RBraceTok, StringRef NamespaceName) {
   assert(hasEndComment(RBraceTok));
   const FormatToken *Comment = RBraceTok->Next;
   SmallVector<StringRef, 7> Groups;
for (size_t I = 0, E = AnnotatedLines.size(); I != E; ++I) {
     const FormatToken *RBraceTok = EndLine->First;
     const std::string NamespaceName = computeName(NamespaceTok);
     bool AddNewline = (I + 1 < E) &&
                       AnnotatedLines[I + 1]->First->NewlinesBefore == 0 &&
                       AnnotatedLines[I + 1]->First->isNot(tok::eof);
     const std::string EndCommentText =
         computeEndCommentText(NamespaceName, AddNewline);
     if (!hasEndComment(RBraceTok)) {
-      if (!isShort(NamespaceTok, RBraceTok, SourceMgr))
+      bool isShort = I - StartLineIndex <= kShortNamespaceMaxLines + 1;
+      if (!isShort)
         addEndComment(RBraceTok, EndCommentText, SourceMgr, &Fixes);
       continue;
     }
     if (!validEndComment(RBraceTok, NamespaceName))
       updateEndComment(RBraceTok, EndCommentText, SourceMgr, &Fixes);
   }
   return Fixes;
 }
 
 } // namespace format
 } // namespace clang

unittests/Format/FormatTest.cpp

EXPECT_EQ("class C {\n"
                    " int i;\n"
                    "\n"
                    "};"));
 
   // Don't remove empty lines at the start of namespaces or extern "C" blocks.
   EXPECT_EQ("namespace N {\n"
             "\n"
             "int i;\n"
-            "}  // namespace N",
+            "}",
             format("namespace N {\n"
                    "\n"
                    "int    i;\n"
                    "}",
                    getGoogleStyle()));
   EXPECT_EQ("extern /**/ \"C\" /**/ {\n"
             "\n"
             "int i;\n"
EXPECT_EQ("namespace {\n"
             "int i;\n"
             "}",
             format("namespace {\n"
                    "int i;\n"
                    "\n"
                    "}", LLVMWithNoNamespaceFix));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
-            "\n"
-            "} // namespace",
+            "}",
             format("namespace {\n"
                    "int i;\n"
                    "\n"
                    "}"));
   EXPECT_EQ("namespace {\n"
             "int i;\n"
             "\n"
             "} // namespace",
EXPECT_EQ("void f() {\n"
             format("void  f  (  )  {  if  ( a )  return  }"));
   EXPECT_EQ("namespace N {\n"
             "void f()\n"
             "}",
             format("namespace  N  {  void f()  }"));
   EXPECT_EQ("namespace N {\n"
             "void f() {}\n"
             "void g()\n"
-            "}",
+            "} // namespace N",
             format("namespace N  { void f( ) { } void g( ) }"));
 }
 
 TEST_F(FormatTest, IndentationWithinColumnLimitNotPossible) {
   verifyFormat("int aaaaaaaa =\n"
                "    // Overlylongcomment\n"
                "    b;",
                getLLVMStyleWithColumns(20));
TEST_F(FormatTest, FormatStarDependingOnContext) {
   verifyFormat("class A {\n  void f(int *a);\n};");
   verifyFormat("class A {\n  int *a;\n};");
   verifyFormat("namespace a {\n"
                "namespace b {\n"
                "class A {\n"
                "  void f() {}\n"
                "  int *a;\n"
                "};\n"
-               "}\n"
-               "}");
+               "} // namespace b\n"
+               "} // namespace a");
 }
 
 TEST_F(FormatTest, SpecialTokensAtEndOfLine) {
   verifyFormat("while");
   verifyFormat("operator");
 }
 
 TEST_F(FormatTest, SkipsDeeplyNestedLines) {
verifyFormat("namespace a {\n"
                "      b();\n"
                "    }\n"
                "  }\n"
                "  void g() { return; }\n"
                "};\n"
                "struct B {\n"
                "  int x;\n"
                "};\n"
-               "}\n",
+               "} // namespace a\n",
                StroustrupBraceStyle);
 
   verifyFormat("void foo()\n"
                "{\n"
                "  if (a) {\n"
                "    a();\n"
                "  }\n"
                "  else {\n"

unittests/Format/NamespaceEndCommentsFixerTest.cpp

return fixNamespaceEndComments(
         Code,
         /*Ranges=*/{1, tooling::Range(0, Code.size())}, Style);
   }
 };
 
 TEST_F(NamespaceEndCommentsFixerTest, AddsEndComment) {
   EXPECT_EQ("namespace {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace",
             fixNamespaceEndComments("namespace {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}"));
   EXPECT_EQ("namespace {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace\n",
             fixNamespaceEndComments("namespace {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}\n"));
   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("inline namespace A {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace A",
             fixNamespaceEndComments("inline namespace A {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}"));
   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("namespace ::A::B {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace ::A::B",
             fixNamespaceEndComments("namespace ::A::B {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}"));
   EXPECT_EQ("namespace /**/::/**/A/**/::/**/B/**/ {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace ::A::B",
             fixNamespaceEndComments("namespace /**/::/**/A/**/::/**/B/**/ {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "namespace B {\n"
             "  int i;\n"
+            "}\n"
+            "}// namespace A",
+            fixNamespaceEndComments("namespace A {\n"
+                                    "namespace B {\n"
+                                    "  int i;\n"
+                                    "}\n"
+                                    "}"));
+  EXPECT_EQ("namespace A {\n"
+            "namespace B {\n"
+            "  int i;\n"
+            "  int j;\n"
             "}// namespace B\n"
             "}// namespace A",
             fixNamespaceEndComments("namespace A {\n"
                                     "namespace B {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "}\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "  int a;\n"
+            "  int b;\n"
             "}// namespace A\n"
             "namespace B {\n"
             "  int b;\n"
+            "  int a;\n"
             "}// namespace B",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int a;\n"
+                                    "  int b;\n"
                                     "}\n"
                                     "namespace B {\n"
                                     "  int b;\n"
+                                    "  int a;\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "  int a1;\n"
+            "  int a2;\n"
             "}// namespace A\n"
             "namespace A {\n"
             "  int a2;\n"
+            "  int a1;\n"
             "}// namespace A",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int a1;\n"
+                                    "  int a2;\n"
                                     "}\n"
                                     "namespace A {\n"
                                     "  int a2;\n"
+                                    "  int a1;\n"
                                     "}"));
   EXPECT_EQ("namespace A {\n"
             "  int a;\n"
+            "  int b;\n"
             "}// namespace A\n"
             "// comment about b\n"
             "int b;",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int a;\n"
+                                    "  int b;\n"
                                     "}\n"
                                     "// comment about b\n"
                                     "int b;"));
 
   EXPECT_EQ("namespace A {\n"
             "namespace B {\n"
             "namespace C {\n"
             "namespace D {\n"
-            "}// namespace D\n"
+            "}\n"
             "}// namespace C\n"
             "}// namespace B\n"
             "}// namespace A",
             fixNamespaceEndComments("namespace A {\n"
                                     "namespace B {\n"
                                     "namespace C {\n"
                                     "namespace D {\n"
                                     "}\n"
                                     "}\n"
                                     "}\n"
                                     "}"));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, AddsNewlineIfNeeded) {
   EXPECT_EQ("namespace A {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace A\n"
-            " int j;",
+            " int k;",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"
-                                    "} int j;"));
+                                    "  int j;\n"
+                                    "} int k;"));
   EXPECT_EQ("namespace {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace\n"
-            " int j;",
+            " int k;",
             fixNamespaceEndComments("namespace {\n"
                                     "  int i;\n"
-                                    "} int j;"));
+                                    "  int j;\n"
+                                    "} int k;"));
   EXPECT_EQ("namespace A {\n"
             "  int i;\n"
+            "  int j;\n"
             "}// namespace A\n"
             " namespace B {\n"
             "  int j;\n"
+            "  int k;\n"
             "}// namespace B",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"
+                                    "  int j;\n"
                                     "} namespace B {\n"
                                     "  int j;\n"
+                                    "  int k;\n"
                                     "}"));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddEndCommentForShortNamespace) {
   EXPECT_EQ("namespace {}", fixNamespaceEndComments("namespace {}"));
   EXPECT_EQ("namespace A {}", fixNamespaceEndComments("namespace A {}"));
-  EXPECT_EQ("namespace A { int i; }",
-            fixNamespaceEndComments("namespace A { int i; }"));
+  EXPECT_EQ("namespace A { a }",
+            fixNamespaceEndComments("namespace A { a }"));
 }
 
 TEST_F(NamespaceEndCommentsFixerTest, DoesNotAddCommentAfterUnaffectedRBrace) {
   EXPECT_EQ("namespace A {\n"
             "  int i;\n"
             "}",
             fixNamespaceEndComments("namespace A {\n"
                                     "  int i;\n"