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

[via cfe-commits]

Author: mydeveloperday
Date: 2020-05-15T21:00:55+01:00
New Revision: e8ea35e63f50486fe497d8565abb8cd5b2820a96

URL: https://github.com/llvm/llvm-project/commit/e8ea35e63f50486fe497d8565abb8cd5b2820a96
DIFF: https://github.com/llvm/llvm-project/commit/e8ea35e63f50486fe497d8565abb8cd5b2820a96.diff

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

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

Reviewed By: krasimir

Subscribers: cfe-commits

Tags: #clang, #clang-format

Differential Revision: https://reviews.llvm.org/D79935

Added: 
    

Modified: 
    clang/lib/Format/NamespaceEndCommentsFixer.cpp
    clang/unittests/Format/FormatTest.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Format/NamespaceEndCommentsFixer.cpp b/clang/lib/Format/NamespaceEndCommentsFixer.cpp
index 20b424f86077..92707150fcdb 100644
--- a/clang/lib/Format/NamespaceEndCommentsFixer.cpp
+++ b/clang/lib/Format/NamespaceEndCommentsFixer.cpp
@@ -121,7 +121,25 @@ bool validEndComment(const FormatToken *RBraceTok, StringRef NamespaceName,
   // 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,

diff  --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index f6f4ea825086..a421ca9f131c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -16238,6 +16238,74 @@ TEST_F(FormatTest, OperatorSpacing) {
   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