[clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine

[galenelias]

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.

[llvmbot]

@llvm/pr-subscribers-clang-format

Author: Galen Elias (galenelias)

Changes

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 --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) {

[mydeveloperday]

Is there an github "Issue" for this? I can't quite understand what problem we are trying to fix.

[galenelias]

Is there an github "Issue" for this? I can't quite understand what problem we are trying to fix.

There is not a GitHub Issue filed yet, but I could get one filed if that would be helpful.

The issue is just there are users who want to use AllowShortNamespacesOnASingleLine=true, but who also currently use BraceWrapping.AfterNamespace=true.

So, they want the following code:

namespace Foo
{
struct Bar;
}

To be formatted as:

namespace Foo { struct Bar; }

But currently that isn't working. AllowShortNamespacesOnASingleLine is currently written assuming BraceWrapping.AfterNamespace == false (which is the more common style).

[owenca]

Please fix the warning:

clang/lib/Format/UnwrappedLineFormatter.cpp:681:31: warning: comparison of integers of different signs: 'typename iterator_traits<AnnotatedLine *const *>::difference_type' (aka 'long') and 'const size_t' (aka 'const unsigned long') [-Wsign-compare]
  681 |       if (std::distance(I, E) <= N)
      |           ~~~~~~~~~~~~~~~~~~~ ^  ~

[ActuallyaDeviloper]

Just this moment I was about to open an issue about this, then I was looking for the commit to reference that introduced the feature and found this pull request. I really hope this can be fixed soon.