[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-format

Author: Galen Elias (galenelias)

<details>
<summary>Changes</summary>

AllowShortNamespacesOnASingleLine assumes that there is no newline before the namespace brace, however, there is no actual reason this shouldn't be compatible with BraceWrapping.AfterNamespace = true.

This is a little tricky in the implementation because UnwrappedLineFormatter works on lines, so being flexible about the offsets is awkward.

Not sure if there is a better pattern for combining the 'AllowShort' options with the various configurations of BraceWrapping, but this seemed mostly reasonable.  Really, it would almost be preferable to just pattern match on the direct token stream, rather than the AnnotatedLines, but I'm not seeing a straightforward way to do that.

---
Full diff: https://github.com/llvm/llvm-project/pull/123010.diff


2 Files Affected:

- (modified) clang/lib/Format/UnwrappedLineFormatter.cpp (+29-16) 
- (modified) clang/unittests/Format/FormatTest.cpp (+20) 


``````````diff
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191abb..787136a26b378e 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -367,8 +367,12 @@ class LineJoiner {
 
     if (Style.AllowShortNamespacesOnASingleLine &&
         TheLine->First->is(tok::kw_namespace) &&
-        TheLine->Last->is(tok::l_brace)) {
-      const auto result = tryMergeNamespace(I, E, Limit);
+        ((Style.BraceWrapping.AfterNamespace &&
+          NextLine.First->is(tok::l_brace)) ||
+         (!Style.BraceWrapping.AfterNamespace &&
+          TheLine->Last->is(tok::l_brace)))) {
+      const auto result =
+          tryMergeNamespace(I, E, Limit, Style.BraceWrapping.AfterNamespace);
       if (result > 0)
         return result;
     }
@@ -628,28 +632,36 @@ class LineJoiner {
 
   unsigned tryMergeNamespace(ArrayRef<AnnotatedLine *>::const_iterator I,
                              ArrayRef<AnnotatedLine *>::const_iterator E,
-                             unsigned Limit) {
+                             unsigned Limit, bool OpenBraceWrapped) {
     if (Limit == 0)
       return 0;
 
-    assert(I[1]);
-    const auto &L1 = *I[1];
+    // The merging code is relative to the opening namespace brace, which could
+    // be either on the first or second line due to the brace wrapping rules.
+    const size_t OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
+    const auto BraceOpenLine = I + OpeningBraceLineOffset;
+
+    if (std::distance(BraceOpenLine, E) <= 2)
+      return 0;
+
+    if (BraceOpenLine[0]->Last->is(TT_LineComment))
+      return 0;
+
+    assert(BraceOpenLine[1]);
+    const auto &L1 = *BraceOpenLine[1];
     if (L1.InPPDirective != (*I)->InPPDirective ||
         (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
       return 0;
     }
 
-    if (std::distance(I, E) <= 2)
-      return 0;
-
-    assert(I[2]);
-    const auto &L2 = *I[2];
+    assert(BraceOpenLine[2]);
+    const auto &L2 = *BraceOpenLine[2];
     if (L2.Type == LT_Invalid)
       return 0;
 
     Limit = limitConsideringMacros(I + 1, E, Limit);
 
-    if (!nextTwoLinesFitInto(I, Limit))
+    if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
       return 0;
 
     // Check if it's a namespace inside a namespace, and call recursively if so.
@@ -660,17 +672,18 @@ class LineJoiner {
 
       assert(Limit >= L1.Last->TotalLength + 3);
       const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
-      const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+      const auto MergedLines =
+          tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, OpenBraceWrapped);
       if (MergedLines == 0)
         return 0;
-      const auto N = MergedLines + 2;
+      const auto N = MergedLines + 2 + OpeningBraceLineOffset;
       // Check if there is even a line after the inner result.
       if (std::distance(I, E) <= N)
         return 0;
       // Check that the line after the inner result starts with a closing brace
       // which we are permitted to merge into one line.
       if (I[N]->First->is(tok::r_brace) && !I[N]->First->MustBreakBefore &&
-          I[MergedLines + 1]->Last->isNot(tok::comment) &&
+          BraceOpenLine[MergedLines + 1]->Last->isNot(tok::comment) &&
           nextNLinesFitInto(I, I + N + 1, Limit)) {
         return N;
       }
@@ -688,8 +701,8 @@ class LineJoiner {
     if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
       return 0;
 
-    // If so, merge all three lines.
-    return 2;
+    // If so, merge all lines.
+    return 2 + OpeningBraceLineOffset;
   }
 
   unsigned
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 4d48bcacddead8..bf008e61490f57 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28430,6 +28430,26 @@ TEST_F(FormatTest, ShortNamespacesOption) {
       "}}} // namespace foo::bar::baz",
       "namespace foo { namespace bar { namespace baz { class qux; } } }",
       Style);
+  Style.FixNamespaceComments = false;
+
+  Style.BreakBeforeBraces = FormatStyle::BS_Custom;
+  Style.BraceWrapping.AfterNamespace = true;
+  verifyFormat("namespace foo { class bar; }", Style);
+  verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+  verifyFormat("namespace foo\n"
+               "{ // comment\n"
+               "class bar;\n"
+               "}",
+               Style);
+
+  verifyFormat("namespace foo\n"
+               "{\n"
+               "namespace bar\n"
+               "{ // comment\n"
+               "class baz;\n"
+               "}\n"
+               "}\n",
+               Style);
 }
 
 TEST_F(FormatTest, WrapNamespaceBodyWithEmptyLinesNever) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/123010