[clang-format] [PR44345] Long namespace closing comment is duplicated endlessly

[MyDeveloperDay]

Summary

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

When namespaces get long the namespace end comment wraps onto the next line

namespace would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::
    went::mad::now {
void foo();
void bar();
} // namespace
  // would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::went::mad::now

If clang-format it applied successively it will duplicate the end comment

namespace would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::
    went::mad::now {
void foo();
void bar();
} // namespace
  // would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::went::mad::now
  // would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::went::mad::now

This revision checks to ensure the end comment is not on the next line before adding yet another comment

[krasimir]

Awesome! Thank you!

[curdeius]

clang/lib/Format/NamespaceEndCommentsFixer.cpp

129

Nit: typo commenline -> commentline.

[MyDeveloperDay]

Fix typo

clang/lib/Format/NamespaceEndCommentsFixer.cpp

bool validEndComment(const FormatToken *RBraceTok, StringRef NamespaceName,
   StringRef NamespaceNameInComment = Groups.size() > 5 ? Groups[5] : "";
   // Anonymous namespace comments must not mention a namespace name.
   if (NamespaceName.empty() && !NamespaceNameInComment.empty())
     return false;
   StringRef AnonymousInComment = Groups.size() > 3 ? Groups[3] : "";
   // Named namespace comments must not mention anonymous namespace.
   if (!NamespaceName.empty() && !AnonymousInComment.empty())
     return false;
-  return NamespaceNameInComment == NamespaceName;
+  if (NamespaceNameInComment == NamespaceName)
+    return true;
+
+  // Has namespace comment flowed onto the next line.
+  // } // namespace
+  //   // verylongnamespacenamethatdidnotfitonthepreviouscommentline
+  if (!(Comment->Next && Comment->Next->is(TT_LineComment)))
+    return false;
+
+  static const llvm::Regex CommentPattern = llvm::Regex(
+      "^/[/*] *( +([a-zA-Z0-9:_]+))?\\.? *(\\*/)?$", llvm::Regex::IgnoreCase);
+
+  // Pull out just the comment text.
+  if (!CommentPattern.match(Comment->Next->TokenText, &Groups)) {
+    return false;
+  }
+  NamespaceNameInComment = Groups.size() > 2 ? Groups[2] : "";
+
+  return (NamespaceNameInComment == NamespaceName);
 }
 
 void addEndComment(const FormatToken *RBraceTok, StringRef EndCommentText,
                    const SourceManager &SourceMgr,
                    tooling::Replacements *Fixes) {
   auto EndLoc = RBraceTok->Tok.getEndLoc();
   auto Range = CharSourceRange::getCharRange(EndLoc, EndLoc);
   auto Err = Fixes->add(tooling::Replacement(SourceMgr, Range, EndCommentText));

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, OperatorSpacing) {
   verifyFormat("Foo::operator&&();", Style);
   verifyFormat("Foo::operator void &&();", Style);
   verifyFormat("Foo::operator()(void &&);", Style);
   verifyFormat("Foo::operator&&(void &&);", Style);
   verifyFormat("Foo::operator&&();", Style);
   verifyFormat("operator&&(int(&&)(), class Foo);", Style);
 }
 
+TEST_F(FormatTest, VeryLongNamespaceCommentSplit) {
+  // These tests are not in NamespaceFixer because that doesn't
+  // test its interaction with line wrapping
+  FormatStyle Style = getLLVMStyle();
+  Style.ColumnLimit = 80;
+  verifyFormat("namespace {\n"
+               "int i;\n"
+               "int j;\n"
+               "} // namespace",
+               Style);
+
+  verifyFormat("namespace AAA {\n"
+               "int i;\n"
+               "int j;\n"
+               "} // namespace AAA",
+               Style);
+
+  EXPECT_EQ("namespace Averyveryveryverylongnamespace {\n"
+            "int i;\n"
+            "int j;\n"
+            "} // namespace Averyveryveryverylongnamespace",
+            format("namespace Averyveryveryverylongnamespace {\n"
+                   "int i;\n"
+                   "int j;\n"
+                   "}",
+                   Style));
+
+  EXPECT_EQ(
+      "namespace "
+      "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::\n"
+      "    went::mad::now {\n"
+      "int i;\n"
+      "int j;\n"
+      "} // namespace\n"
+      "  // "
+      "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::"
+      "went::mad::now",
+      format("namespace "
+             "would::it::save::you::a::lot::of::time::if_::i::"
+             "just::gave::up::and_::went::mad::now {\n"
+             "int i;\n"
+             "int j;\n"
+             "}",
+             Style));
+
+  // This used to duplicate the comment again and again on subsequent runs
+  EXPECT_EQ(
+      "namespace "
+      "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::\n"
+      "    went::mad::now {\n"
+      "int i;\n"
+      "int j;\n"
+      "} // namespace\n"
+      "  // "
+      "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::and_::"
+      "went::mad::now",
+      format("namespace "
+             "would::it::save::you::a::lot::of::time::if_::i::"
+             "just::gave::up::and_::went::mad::now {\n"
+             "int i;\n"
+             "int j;\n"
+             "} // namespace\n"
+             "  // "
+             "would::it::save::you::a::lot::of::time::if_::i::just::gave::up::"
+             "and_::went::mad::now",
+             Style));
+}
+
 } // namespace
 } // namespace format
 } // namespace clang