[clang] [clang-format] Add `AllowShortNamespacesOnASingleLine` option (PR #105597)
Owen Pan via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 30 01:01:23 PST 2024
https://github.com/owenca updated https://github.com/llvm/llvm-project/pull/105597
>From 93eb3d89652607173f4f68fce7dcc5b2bd33f266 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Wed, 21 Aug 2024 16:33:42 -0700
Subject: [PATCH 01/18] clang-format: Add "AllowShortNamespacesOnASingleLine"
option
This addresses: https://github.com/llvm/llvm-project/issues/101363
which is a resurrection of a previously opened but never completed
review: https://reviews.llvm.org/D11851
The feature is to allow code like the following not to be broken across
multiple lines:
```
namespace foo { class bar; }
namespace foo { namespace bar { class baz; } }
```
Code like this is commonly used for forward declarations, which are
ideally kept compact. This is also apparently the format that
include-what-you-use will insert for forward declarations.
---
clang/include/clang/Format/Format.h | 5 +
clang/lib/Format/Format.cpp | 3 +
clang/lib/Format/UnwrappedLineFormatter.cpp | 82 ++++++++++++++
clang/unittests/Format/FormatTest.cpp | 112 ++++++++++++++++++++
4 files changed, 202 insertions(+)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 6383934afa2c40..26cd673685942e 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,6 +988,11 @@ struct FormatStyle {
/// \version 3.7
bool AllowShortLoopsOnASingleLine;
+ /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
+ /// line.
+ /// \version 19
+ bool AllowShortNamespacesOnASingleLine;
+
/// Different ways to break after the function definition return type.
/// This option is **deprecated** and is retained for backwards compatibility.
enum DefinitionReturnTypeBreakingStyle : int8_t {
diff --git a/clang/lib/Format/Format.cpp b/clang/lib/Format/Format.cpp
index 95129a8fe9240c..8f44e9f00212cf 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -975,6 +975,8 @@ template <> struct MappingTraits<FormatStyle> {
Style.AllowShortLambdasOnASingleLine);
IO.mapOptional("AllowShortLoopsOnASingleLine",
Style.AllowShortLoopsOnASingleLine);
+ IO.mapOptional("AllowShortNamespacesOnASingleLine",
+ Style.AllowShortNamespacesOnASingleLine);
IO.mapOptional("AlwaysBreakAfterDefinitionReturnType",
Style.AlwaysBreakAfterDefinitionReturnType);
IO.mapOptional("AlwaysBreakBeforeMultilineStrings",
@@ -1480,6 +1482,7 @@ FormatStyle getLLVMStyle(FormatStyle::LanguageKind Language) {
LLVMStyle.AllowShortIfStatementsOnASingleLine = FormatStyle::SIS_Never;
LLVMStyle.AllowShortLambdasOnASingleLine = FormatStyle::SLS_All;
LLVMStyle.AllowShortLoopsOnASingleLine = false;
+ LLVMStyle.AllowShortNamespacesOnASingleLine = false;
LLVMStyle.AlwaysBreakAfterDefinitionReturnType = FormatStyle::DRTBS_None;
LLVMStyle.AlwaysBreakBeforeMultilineStrings = false;
LLVMStyle.AttributeMacros.push_back("__capability");
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 1804c1437fd41d..971eac1978bb71 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -420,6 +420,15 @@ class LineJoiner {
TheLine->First != LastNonComment) {
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
+
+ if (TheLine->Last->is(tok::l_brace)) {
+ if (Style.AllowShortNamespacesOnASingleLine &&
+ TheLine->First->is(tok::kw_namespace)) {
+ if (unsigned result = tryMergeNamespace(I, E, Limit))
+ return result;
+ }
+ }
+
// Try to merge a control statement block with left brace unwrapped.
if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
@@ -616,6 +625,62 @@ class LineJoiner {
return 1;
}
+ unsigned tryMergeNamespace(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
+ SmallVectorImpl<AnnotatedLine *>::const_iterator E,
+ unsigned Limit) {
+ if (Limit == 0)
+ return 0;
+ if (I[1]->InPPDirective != (*I)->InPPDirective ||
+ (I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+ return 0;
+ }
+ if (I + 2 == E || I[2]->Type == LT_Invalid)
+ return 0;
+
+ Limit = limitConsideringMacros(I + 1, E, Limit);
+
+ if (!nextTwoLinesFitInto(I, Limit))
+ return 0;
+
+ // Check if it's a namespace inside a namespace, and call recursively if so
+ // '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+ if (I[1]->First->is(tok::kw_namespace)) {
+ if (I[1]->Last->is(TT_LineComment))
+ return 0;
+
+ unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
+ unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
+ if (!inner_result)
+ return 0;
+ // check if there is even a line after the inner result
+ if (I + 2 + inner_result >= E)
+ 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[2 + inner_result]->First->is(tok::r_brace) &&
+ !I[2 + inner_result]->First->MustBreakBefore &&
+ !I[1 + inner_result]->Last->is(TT_LineComment) &&
+ nextNLinesFitInto(I, I + 2 + inner_result + 1, Limit)) {
+ return 2 + inner_result;
+ }
+ return 0;
+ }
+
+ // There's no inner namespace, so we are considering to merge at most one
+ // line.
+
+ // The line which is in the namespace should end with semicolon
+ if (I[1]->Last->isNot(tok::semi))
+ return 0;
+
+ // Last, check that the third line starts with a closing brace.
+ if (I[2]->First->isNot(tok::r_brace) || I[2]->First->MustBreakBefore)
+ return 0;
+
+ // If so, merge all three lines.
+ return 2;
+ }
+
unsigned tryMergeSimpleControlStatement(
SmallVectorImpl<AnnotatedLine *>::const_iterator I,
SmallVectorImpl<AnnotatedLine *>::const_iterator E, unsigned Limit) {
@@ -916,6 +981,23 @@ class LineJoiner {
return 1 + I[1]->Last->TotalLength + 1 + I[2]->Last->TotalLength <= Limit;
}
+ bool nextNLinesFitInto(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
+ SmallVectorImpl<AnnotatedLine *>::const_iterator E,
+ unsigned Limit) {
+ unsigned joinedLength = 0;
+ for (SmallVectorImpl<AnnotatedLine *>::const_iterator J = I + 1; J != E;
+ ++J) {
+
+ if ((*J)->First->MustBreakBefore)
+ return false;
+
+ joinedLength += 1 + (*J)->Last->TotalLength;
+ if (joinedLength > Limit)
+ return false;
+ }
+ return true;
+ }
+
bool containsMustBreak(const AnnotatedLine *Line) {
assert(Line->First);
// Ignore the first token, because in this situation, it applies more to the
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 47465a18e9a41e..8535a167c31b1c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28314,6 +28314,118 @@ TEST_F(FormatTest, KeepFormFeed) {
Style);
}
+TEST_F(FormatTest, ShortNamespacesOption) {
+ FormatStyle style = getLLVMStyle();
+ style.AllowShortNamespacesOnASingleLine = true;
+ style.FixNamespaceComments = false;
+
+ // Basic functionality
+ verifyFormat("namespace foo { class bar; }", style);
+ verifyFormat("namespace foo::bar { class baz; }", style);
+ verifyFormat("namespace { class bar; }", style);
+ verifyFormat("namespace foo {\n"
+ "class bar;\n"
+ "class baz;\n"
+ "}",
+ style);
+
+ // Trailing comments prevent merging
+ verifyFormat("namespace foo {\n"
+ "namespace baz { class qux; } // comment\n"
+ "}",
+ style);
+
+ // Make sure code doesn't walk to far on unbalanced code
+ verifyFormat("namespace foo {", style);
+ verifyFormat("namespace foo {\n"
+ "class baz;",
+ style);
+ verifyFormat("namespace foo {\n"
+ "namespace bar { class baz; }",
+ style);
+
+ // Nested namespaces
+ verifyFormat("namespace foo { namespace bar { class baz; } }", style);
+ verifyFormat("namespace foo {\n"
+ "namespace bar { class baz; }\n"
+ "namespace quar { class quaz; }\n"
+ "}",
+ style);
+
+ // Varying inner content
+ verifyFormat("namespace foo {\n"
+ "int f() { return 5; }\n"
+ "}",
+ style);
+ verifyFormat("namespace foo { template <T> struct bar; }", style);
+ verifyFormat("namespace foo { constexpr int num = 42; }", style);
+
+ // Validate wrapping scenarios around the ColumnLimit
+ style.ColumnLimit = 64;
+
+ // Validate just under the ColumnLimit
+ verifyFormat(
+ "namespace foo { namespace bar { namespace baz { class qux; } } }",
+ style);
+
+ // Validate just over the ColumnLimit
+ verifyFormat("namespace foo {\n"
+ "namespace bar { namespace baz { class quux; } }\n"
+ "}",
+ style);
+
+ verifyFormat("namespace foo {\n"
+ "namespace bar {\n"
+ "namespace baz { namespace qux { class quux; } }\n"
+ "}\n"
+ "}",
+ style);
+
+ // Validate that the ColumnLimit logic accounts for trailing content as well
+ verifyFormat("namespace foo {\n"
+ "namespace bar { namespace baz { class qux; } }\n"
+ "} // extra",
+ style);
+
+ // No ColumnLimit, allows long nested one-liners, but also leaves multi-line
+ // instances alone
+ style.ColumnLimit = 0;
+ verifyFormat("namespace foo { namespace bar { namespace baz { namespace qux "
+ "{ class quux; } } } }",
+ style);
+
+ verifyNoChange("namespace foo {\n"
+ "namespace bar {\n"
+ "namespace baz { namespace qux { class quux; } }\n"
+ "}\n"
+ "}",
+ style);
+
+ // This option doesn't really work with FixNamespaceComments and nested
+ // namespaces. Code should use the concatenated namespace syntax. e.g.
+ // 'namespace foo::bar'
+ style.FixNamespaceComments = true;
+
+ verifyFormat(
+ "namespace foo {\n"
+ "namespace bar { namespace baz { class qux; } } // namespace bar\n"
+ "} // namespace foo",
+ "namespace foo { namespace bar { namespace baz { class qux; } } }",
+ style);
+
+ // This option doesn't really make any sense with ShortNamespaceLines = 0
+ style.ShortNamespaceLines = 0;
+
+ verifyFormat(
+ "namespace foo {\n"
+ "namespace bar {\n"
+ "namespace baz { class qux; } // namespace baz\n"
+ "} // namespace bar\n"
+ "} // namespace foo",
+ "namespace foo { namespace bar { namespace baz { class qux; } } }",
+ style);
+}
+
} // namespace
} // namespace test
} // namespace format
>From 04bd15bf1201c33773a89514f558089143afb9e5 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Thu, 22 Aug 2024 09:16:32 -0700
Subject: [PATCH 02/18] Use PascalCase for local variable names
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 18 +++----
clang/unittests/Format/FormatTest.cpp | 56 ++++++++++-----------
2 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 971eac1978bb71..ddb94c62d0da2c 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -648,20 +648,20 @@ class LineJoiner {
if (I[1]->Last->is(TT_LineComment))
return 0;
- unsigned inner_limit = Limit - I[1]->Last->TotalLength - 3;
- unsigned inner_result = tryMergeNamespace(I + 1, E, inner_limit);
- if (!inner_result)
+ const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
+ const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+ if (!MergedLines)
return 0;
// check if there is even a line after the inner result
- if (I + 2 + inner_result >= E)
+ if (I + 2 + MergedLines >= E)
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[2 + inner_result]->First->is(tok::r_brace) &&
- !I[2 + inner_result]->First->MustBreakBefore &&
- !I[1 + inner_result]->Last->is(TT_LineComment) &&
- nextNLinesFitInto(I, I + 2 + inner_result + 1, Limit)) {
- return 2 + inner_result;
+ if (I[2 + MergedLines]->First->is(tok::r_brace) &&
+ !I[2 + MergedLines]->First->MustBreakBefore &&
+ !I[1 + MergedLines]->Last->is(TT_LineComment) &&
+ nextNLinesFitInto(I, I + 2 + MergedLines + 1, Limit)) {
+ return 2 + MergedLines;
}
return 0;
}
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 8535a167c31b1c..85e7ba3b8f0cd0 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28315,106 +28315,106 @@ TEST_F(FormatTest, KeepFormFeed) {
}
TEST_F(FormatTest, ShortNamespacesOption) {
- FormatStyle style = getLLVMStyle();
- style.AllowShortNamespacesOnASingleLine = true;
- style.FixNamespaceComments = false;
+ FormatStyle Style = getLLVMStyle();
+ Style.AllowShortNamespacesOnASingleLine = true;
+ Style.FixNamespaceComments = false;
// Basic functionality
- verifyFormat("namespace foo { class bar; }", style);
- verifyFormat("namespace foo::bar { class baz; }", style);
- verifyFormat("namespace { class bar; }", style);
+ verifyFormat("namespace foo { class bar; }", Style);
+ verifyFormat("namespace foo::bar { class baz; }", Style);
+ verifyFormat("namespace { class bar; }", Style);
verifyFormat("namespace foo {\n"
"class bar;\n"
"class baz;\n"
"}",
- style);
+ Style);
// Trailing comments prevent merging
verifyFormat("namespace foo {\n"
"namespace baz { class qux; } // comment\n"
"}",
- style);
+ Style);
// Make sure code doesn't walk to far on unbalanced code
- verifyFormat("namespace foo {", style);
+ verifyFormat("namespace foo {", Style);
verifyFormat("namespace foo {\n"
"class baz;",
- style);
+ Style);
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }",
- style);
+ Style);
// Nested namespaces
- verifyFormat("namespace foo { namespace bar { class baz; } }", style);
+ verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }\n"
"namespace quar { class quaz; }\n"
"}",
- style);
+ Style);
// Varying inner content
verifyFormat("namespace foo {\n"
"int f() { return 5; }\n"
"}",
- style);
- verifyFormat("namespace foo { template <T> struct bar; }", style);
- verifyFormat("namespace foo { constexpr int num = 42; }", style);
+ Style);
+ verifyFormat("namespace foo { template <T> struct bar; }", Style);
+ verifyFormat("namespace foo { constexpr int num = 42; }", Style);
// Validate wrapping scenarios around the ColumnLimit
- style.ColumnLimit = 64;
+ Style.ColumnLimit = 64;
// Validate just under the ColumnLimit
verifyFormat(
"namespace foo { namespace bar { namespace baz { class qux; } } }",
- style);
+ Style);
// Validate just over the ColumnLimit
verifyFormat("namespace foo {\n"
"namespace bar { namespace baz { class quux; } }\n"
"}",
- style);
+ Style);
verifyFormat("namespace foo {\n"
"namespace bar {\n"
"namespace baz { namespace qux { class quux; } }\n"
"}\n"
"}",
- style);
+ Style);
// Validate that the ColumnLimit logic accounts for trailing content as well
verifyFormat("namespace foo {\n"
"namespace bar { namespace baz { class qux; } }\n"
"} // extra",
- style);
+ Style);
// No ColumnLimit, allows long nested one-liners, but also leaves multi-line
// instances alone
- style.ColumnLimit = 0;
+ Style.ColumnLimit = 0;
verifyFormat("namespace foo { namespace bar { namespace baz { namespace qux "
"{ class quux; } } } }",
- style);
+ Style);
verifyNoChange("namespace foo {\n"
"namespace bar {\n"
"namespace baz { namespace qux { class quux; } }\n"
"}\n"
"}",
- style);
+ Style);
// This option doesn't really work with FixNamespaceComments and nested
// namespaces. Code should use the concatenated namespace syntax. e.g.
// 'namespace foo::bar'
- style.FixNamespaceComments = true;
+ Style.FixNamespaceComments = true;
verifyFormat(
"namespace foo {\n"
"namespace bar { namespace baz { class qux; } } // namespace bar\n"
"} // namespace foo",
"namespace foo { namespace bar { namespace baz { class qux; } } }",
- style);
+ Style);
// This option doesn't really make any sense with ShortNamespaceLines = 0
- style.ShortNamespaceLines = 0;
+ Style.ShortNamespaceLines = 0;
verifyFormat(
"namespace foo {\n"
@@ -28423,7 +28423,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"} // namespace bar\n"
"} // namespace foo",
"namespace foo { namespace bar { namespace baz { class qux; } } }",
- style);
+ Style);
}
} // namespace
>From b00a164b5967d766c96a409387da55e52af22845 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Wed, 28 Aug 2024 10:42:05 -0700
Subject: [PATCH 03/18] Address review comments
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 22 +++++++++----------
clang/unittests/Format/FormatTest.cpp | 24 ++++++++++-----------
2 files changed, 22 insertions(+), 24 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index ddb94c62d0da2c..31bf3f484c4c39 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -643,7 +643,7 @@ class LineJoiner {
return 0;
// Check if it's a namespace inside a namespace, and call recursively if so
- // '3' is the sizes of the whitespace and closing brace for " _inner_ }"
+ // '3' is the sizes of the whitespace and closing brace for " _inner_ }".
if (I[1]->First->is(tok::kw_namespace)) {
if (I[1]->Last->is(TT_LineComment))
return 0;
@@ -652,11 +652,11 @@ class LineJoiner {
const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
if (!MergedLines)
return 0;
- // check if there is even a line after the inner result
- if (I + 2 + MergedLines >= E)
+ // Check if there is even a line after the inner result.
+ if (std::distance(I, E) <= MergedLines + 2)
return 0;
- // check that the line after the inner result starts with a closing brace
- // which we are permitted to merge into one line
+ // Check that the line after the inner result starts with a closing brace
+ // which we are permitted to merge into one line.
if (I[2 + MergedLines]->First->is(tok::r_brace) &&
!I[2 + MergedLines]->First->MustBreakBefore &&
!I[1 + MergedLines]->Last->is(TT_LineComment) &&
@@ -669,7 +669,7 @@ class LineJoiner {
// There's no inner namespace, so we are considering to merge at most one
// line.
- // The line which is in the namespace should end with semicolon
+ // The line which is in the namespace should end with semicolon.
if (I[1]->Last->isNot(tok::semi))
return 0;
@@ -984,15 +984,13 @@ class LineJoiner {
bool nextNLinesFitInto(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
SmallVectorImpl<AnnotatedLine *>::const_iterator E,
unsigned Limit) {
- unsigned joinedLength = 0;
- for (SmallVectorImpl<AnnotatedLine *>::const_iterator J = I + 1; J != E;
- ++J) {
-
+ unsigned JoinedLength = 0;
+ for (auto J = I + 1; J != E; ++J) {
if ((*J)->First->MustBreakBefore)
return false;
- joinedLength += 1 + (*J)->Last->TotalLength;
- if (joinedLength > Limit)
+ JoinedLength += 1 + (*J)->Last->TotalLength;
+ if (JoinedLength > Limit)
return false;
}
return true;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 85e7ba3b8f0cd0..5b326e37602948 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28319,7 +28319,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
Style.AllowShortNamespacesOnASingleLine = true;
Style.FixNamespaceComments = false;
- // Basic functionality
+ // Basic functionality.
verifyFormat("namespace foo { class bar; }", Style);
verifyFormat("namespace foo::bar { class baz; }", Style);
verifyFormat("namespace { class bar; }", Style);
@@ -28329,13 +28329,13 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- // Trailing comments prevent merging
+ // Trailing comments prevent merging.
verifyFormat("namespace foo {\n"
"namespace baz { class qux; } // comment\n"
"}",
Style);
- // Make sure code doesn't walk to far on unbalanced code
+ // Make sure code doesn't walk to far on unbalanced code.
verifyFormat("namespace foo {", Style);
verifyFormat("namespace foo {\n"
"class baz;",
@@ -28344,7 +28344,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"namespace bar { class baz; }",
Style);
- // Nested namespaces
+ // Nested namespaces.
verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }\n"
@@ -28352,7 +28352,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- // Varying inner content
+ // Varying inner content.
verifyFormat("namespace foo {\n"
"int f() { return 5; }\n"
"}",
@@ -28360,15 +28360,15 @@ TEST_F(FormatTest, ShortNamespacesOption) {
verifyFormat("namespace foo { template <T> struct bar; }", Style);
verifyFormat("namespace foo { constexpr int num = 42; }", Style);
- // Validate wrapping scenarios around the ColumnLimit
+ // Validate wrapping scenarios around the ColumnLimit.
Style.ColumnLimit = 64;
- // Validate just under the ColumnLimit
+ // Validate just under the ColumnLimit.
verifyFormat(
"namespace foo { namespace bar { namespace baz { class qux; } } }",
Style);
- // Validate just over the ColumnLimit
+ // Validate just over the ColumnLimit.
verifyFormat("namespace foo {\n"
"namespace bar { namespace baz { class quux; } }\n"
"}",
@@ -28381,14 +28381,14 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- // Validate that the ColumnLimit logic accounts for trailing content as well
+ // Validate that the ColumnLimit logic accounts for trailing content as well.
verifyFormat("namespace foo {\n"
"namespace bar { namespace baz { class qux; } }\n"
"} // extra",
Style);
// No ColumnLimit, allows long nested one-liners, but also leaves multi-line
- // instances alone
+ // instances alone.
Style.ColumnLimit = 0;
verifyFormat("namespace foo { namespace bar { namespace baz { namespace qux "
"{ class quux; } } } }",
@@ -28403,7 +28403,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
// This option doesn't really work with FixNamespaceComments and nested
// namespaces. Code should use the concatenated namespace syntax. e.g.
- // 'namespace foo::bar'
+ // 'namespace foo::bar'.
Style.FixNamespaceComments = true;
verifyFormat(
@@ -28413,7 +28413,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"namespace foo { namespace bar { namespace baz { class qux; } } }",
Style);
- // This option doesn't really make any sense with ShortNamespaceLines = 0
+ // This option doesn't really make any sense with ShortNamespaceLines = 0.
Style.ShortNamespaceLines = 0;
verifyFormat(
>From 8a879290c043b26bacc142af2f2c62d47c4d3e56 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Thu, 5 Sep 2024 07:41:49 -0700
Subject: [PATCH 04/18] Switch version of new format option to 20
---
clang/include/clang/Format/Format.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 26cd673685942e..25486fa9fe847c 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -990,7 +990,7 @@ struct FormatStyle {
/// If ``true``, ``namespace a { class b; }`` can be put on a single a single
/// line.
- /// \version 19
+ /// \version 20
bool AllowShortNamespacesOnASingleLine;
/// Different ways to break after the function definition return type.
>From 228bb3422fcb4a5aac0bcbc86ba59316a0cebf20 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Tue, 10 Sep 2024 08:30:37 -0700
Subject: [PATCH 05/18] Re-generate StylClangFormatStyleOptions.rst
---
clang/docs/ClangFormatStyleOptions.rst | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 4be448171699ca..ff0b03addc1a70 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2088,6 +2088,12 @@ the configuration (without a prefix: ``Auto``).
If ``true``, ``while (true) continue;`` can be put on a single
line.
+.. _AllowShortNamespacesOnASingleLine:
+
+**AllowShortNamespacesOnASingleLine** (``Boolean``) :versionbadge:`clang-format 20` :ref:`¶ <AllowShortNamespacesOnASingleLine>`
+ If ``true``, ``namespace a { class b; }`` can be put on a single a single
+ line.
+
.. _AlwaysBreakAfterDefinitionReturnType:
**AlwaysBreakAfterDefinitionReturnType** (``DefinitionReturnTypeBreakingStyle``) :versionbadge:`clang-format 3.7` :ref:`¶ <AlwaysBreakAfterDefinitionReturnType>`
>From a2462023ca98b976d44a96adeb3baede505fd117 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Tue, 10 Sep 2024 08:31:06 -0700
Subject: [PATCH 06/18] Add ConfigParseTest
---
clang/unittests/Format/ConfigParseTest.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang/unittests/Format/ConfigParseTest.cpp b/clang/unittests/Format/ConfigParseTest.cpp
index 7fc7492271668b..b249bf073aa453 100644
--- a/clang/unittests/Format/ConfigParseTest.cpp
+++ b/clang/unittests/Format/ConfigParseTest.cpp
@@ -159,6 +159,7 @@ TEST(ConfigParseTest, ParsesConfigurationBools) {
CHECK_PARSE_BOOL(AllowShortCompoundRequirementOnASingleLine);
CHECK_PARSE_BOOL(AllowShortEnumsOnASingleLine);
CHECK_PARSE_BOOL(AllowShortLoopsOnASingleLine);
+ CHECK_PARSE_BOOL(AllowShortNamespacesOnASingleLine);
CHECK_PARSE_BOOL(BinPackArguments);
CHECK_PARSE_BOOL(BreakAdjacentStringLiterals);
CHECK_PARSE_BOOL(BreakAfterJavaFieldAnnotations);
>From 22967da796141e23f91451cb8ca01e3723ec5ac2 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Tue, 10 Sep 2024 08:31:21 -0700
Subject: [PATCH 07/18] Fix interaction with CompactNamespaces
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 17 +++++-----
clang/unittests/Format/FormatTest.cpp | 36 ++++++++++++++-------
2 files changed, 34 insertions(+), 19 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 31bf3f484c4c39..756278b4e8ddf4 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -361,9 +361,18 @@ class LineJoiner {
const auto *FirstNonComment = TheLine->getFirstNonComment();
if (!FirstNonComment)
return 0;
+
// FIXME: There are probably cases where we should use FirstNonComment
// instead of TheLine->First.
+ if (TheLine->Last->is(tok::l_brace)) {
+ if (Style.AllowShortNamespacesOnASingleLine &&
+ TheLine->First->is(tok::kw_namespace)) {
+ if (unsigned result = tryMergeNamespace(I, E, Limit))
+ return result;
+ }
+ }
+
if (Style.CompactNamespaces) {
if (const auto *NSToken = TheLine->First->getNamespaceToken()) {
int J = 1;
@@ -421,14 +430,6 @@ class LineJoiner {
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
- if (TheLine->Last->is(tok::l_brace)) {
- if (Style.AllowShortNamespacesOnASingleLine &&
- TheLine->First->is(tok::kw_namespace)) {
- if (unsigned result = tryMergeNamespace(I, E, Limit))
- return result;
- }
- }
-
// Try to merge a control statement block with left brace unwrapped.
if (TheLine->Last->is(tok::l_brace) && FirstNonComment != TheLine->Last &&
FirstNonComment->isOneOf(tok::kw_if, tok::kw_while, tok::kw_for,
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 5b326e37602948..8f4eb4ba5edce5 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28315,9 +28315,11 @@ TEST_F(FormatTest, KeepFormFeed) {
}
TEST_F(FormatTest, ShortNamespacesOption) {
- FormatStyle Style = getLLVMStyle();
- Style.AllowShortNamespacesOnASingleLine = true;
- Style.FixNamespaceComments = false;
+ auto BaseStyle = getLLVMStyle();
+ BaseStyle.AllowShortNamespacesOnASingleLine = true;
+ BaseStyle.FixNamespaceComments = false;
+
+ auto Style = BaseStyle;
// Basic functionality.
verifyFormat("namespace foo { class bar; }", Style);
@@ -28335,7 +28337,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- // Make sure code doesn't walk to far on unbalanced code.
+ // Make sure code doesn't walk too far on unbalanced code.
verifyFormat("namespace foo {", Style);
verifyFormat("namespace foo {\n"
"class baz;",
@@ -28348,7 +28350,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }\n"
- "namespace quar { class quaz; }\n"
+ "namespace qux { class quux; }\n"
"}",
Style);
@@ -28390,20 +28392,32 @@ TEST_F(FormatTest, ShortNamespacesOption) {
// No ColumnLimit, allows long nested one-liners, but also leaves multi-line
// instances alone.
Style.ColumnLimit = 0;
- verifyFormat("namespace foo { namespace bar { namespace baz { namespace qux "
- "{ class quux; } } } }",
- Style);
+ verifyFormat(
+ "namespace foo { namespace bar { namespace baz { class qux; } } }",
+ Style);
verifyNoChange("namespace foo {\n"
- "namespace bar {\n"
- "namespace baz { namespace qux { class quux; } }\n"
- "}\n"
+ "namespace bar { namespace baz { class qux; } }\n"
"}",
Style);
+ Style = BaseStyle;
+ Style.CompactNamespaces = true;
+ verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+
+ // If we can't merge an outer nested namespaces, but can merge an inner
+ // nested namespace, then CompactNamespaces will merge the outer namespace
+ // first, preventing the merging of the inner namespace
+ verifyFormat("namespace foo { namespace baz {\n"
+ "class qux;\n"
+ "} // comment\n"
+ "}",
+ Style);
+
// This option doesn't really work with FixNamespaceComments and nested
// namespaces. Code should use the concatenated namespace syntax. e.g.
// 'namespace foo::bar'.
+ Style = BaseStyle;
Style.FixNamespaceComments = true;
verifyFormat(
>From 6548c31848e552ef12be5cf410470a8e2740fd39 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Wed, 25 Sep 2024 16:20:46 -0700
Subject: [PATCH 08/18] Address additional PR comments
---
clang/include/clang/Format/Format.h | 3 +--
clang/lib/Format/UnwrappedLineFormatter.cpp | 20 +++++++++++---------
clang/unittests/Format/FormatTest.cpp | 2 +-
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 25486fa9fe847c..732057334578b6 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,8 +988,7 @@ struct FormatStyle {
/// \version 3.7
bool AllowShortLoopsOnASingleLine;
- /// If ``true``, ``namespace a { class b; }`` can be put on a single a single
- /// line.
+ /// If ``true``, ``namespace a { class b; }`` can be put on a single line.
/// \version 20
bool AllowShortNamespacesOnASingleLine;
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 756278b4e8ddf4..6b57f8005688e9 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -368,7 +368,8 @@ class LineJoiner {
if (TheLine->Last->is(tok::l_brace)) {
if (Style.AllowShortNamespacesOnASingleLine &&
TheLine->First->is(tok::kw_namespace)) {
- if (unsigned result = tryMergeNamespace(I, E, Limit))
+ unsigned result = tryMergeNamespace(I, E, Limit);
+ if (result > 0)
return result;
}
}
@@ -643,26 +644,27 @@ class LineJoiner {
if (!nextTwoLinesFitInto(I, Limit))
return 0;
- // Check if it's a namespace inside a namespace, and call recursively if so
+ // Check if it's a namespace inside a namespace, and call recursively if so.
// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
if (I[1]->First->is(tok::kw_namespace)) {
- if (I[1]->Last->is(TT_LineComment))
+ if (I[1]->Last->is(tok::comment))
return 0;
const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
if (!MergedLines)
return 0;
+ const auto N = MergedLines + 2;
// Check if there is even a line after the inner result.
- if (std::distance(I, E) <= MergedLines + 2)
+ 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[2 + MergedLines]->First->is(tok::r_brace) &&
- !I[2 + MergedLines]->First->MustBreakBefore &&
- !I[1 + MergedLines]->Last->is(TT_LineComment) &&
- nextNLinesFitInto(I, I + 2 + MergedLines + 1, Limit)) {
- return 2 + MergedLines;
+ if (I[N]->First->is(tok::r_brace) &&
+ !I[N]->First->MustBreakBefore &&
+ !I[MergedLines + 1]->Last->is(tok::comment) &&
+ nextNLinesFitInto(I, I + N + 1, Limit)) {
+ return N;
}
return 0;
}
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 8f4eb4ba5edce5..458222a2b6eccc 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28407,7 +28407,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
// If we can't merge an outer nested namespaces, but can merge an inner
// nested namespace, then CompactNamespaces will merge the outer namespace
- // first, preventing the merging of the inner namespace
+ // first, preventing the merging of the inner namespace.
verifyFormat("namespace foo { namespace baz {\n"
"class qux;\n"
"} // comment\n"
>From 0dcdacd1929b517b771d588b829107f492ff9216 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Thu, 26 Sep 2024 08:15:02 -0700
Subject: [PATCH 09/18] clang-format run
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 6b57f8005688e9..300bd770f993da 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -660,8 +660,7 @@ class LineJoiner {
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 &&
+ if (I[N]->First->is(tok::r_brace) && !I[N]->First->MustBreakBefore &&
!I[MergedLines + 1]->Last->is(tok::comment) &&
nextNLinesFitInto(I, I + N + 1, Limit)) {
return N;
>From 72d57e0395ad5a7f32e21467068b1a49b21d970c Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Tue, 15 Oct 2024 14:59:50 -0700
Subject: [PATCH 10/18] Re-work interaction with 'CompactNamespaces' option
- Disable recursive merging of nested namespaces onto a single line
unless 'CompactNamespaces' is enabled.
- Fix off-by-one error in CompactNamespaces code. For nested namespaces
with 3 or more namespaces, it was incorrectly compacting lines which
were 1 or two spaces over the ColumnLimit, leading to incorrect
formatting results.
- Add release notes mention.
- Re-generate Style Options documentation
---
clang/docs/ClangFormatStyleOptions.rst | 3 +-
clang/docs/ReleaseNotes.rst | 2 +
clang/lib/Format/UnwrappedLineFormatter.cpp | 7 +-
clang/unittests/Format/FormatTest.cpp | 98 +++++++++++----------
4 files changed, 60 insertions(+), 50 deletions(-)
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index ff0b03addc1a70..c175436a2817a9 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2091,8 +2091,7 @@ the configuration (without a prefix: ``Auto``).
.. _AllowShortNamespacesOnASingleLine:
**AllowShortNamespacesOnASingleLine** (``Boolean``) :versionbadge:`clang-format 20` :ref:`¶ <AllowShortNamespacesOnASingleLine>`
- If ``true``, ``namespace a { class b; }`` can be put on a single a single
- line.
+ If ``true``, ``namespace a { class b; }`` can be put on a single line.
.. _AlwaysBreakAfterDefinitionReturnType:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 983c1da20ed4c8..1ab0154d71b99c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1122,6 +1122,8 @@ clang-format
- Adds ``RemoveEmptyLinesInUnwrappedLines`` option.
- Adds ``KeepFormFeed`` option and set it to ``true`` for ``GNU`` style.
+- Adds ``AllowShortNamespacesOnASingleLine`` option.
+
libclang
--------
- Add ``clang_isBeforeInTranslationUnit``. Given two source locations, it determines
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 300bd770f993da..d9fa2780034b45 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -383,7 +383,7 @@ class LineJoiner {
ClosingLineIndex == I[J]->MatchingClosingBlockLineIndex &&
I[J]->Last->TotalLength < Limit;
++J, --ClosingLineIndex) {
- Limit -= I[J]->Last->TotalLength;
+ Limit -= I[J]->Last->TotalLength + 1;
// Reduce indent level for bodies of namespaces which were compacted,
// but only if their content was indented in the first place.
@@ -647,9 +647,10 @@ class LineJoiner {
// Check if it's a namespace inside a namespace, and call recursively if so.
// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
if (I[1]->First->is(tok::kw_namespace)) {
- if (I[1]->Last->is(tok::comment))
+ if (I[1]->Last->is(tok::comment) || !Style.CompactNamespaces)
return 0;
+ assert(Limit >= I[1]->Last->TotalLength + 3);
const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
if (!MergedLines)
@@ -661,7 +662,7 @@ class LineJoiner {
// 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->is(tok::comment) &&
+ I[MergedLines + 1]->Last->isNot(tok::comment) &&
nextNLinesFitInto(I, I + N + 1, Limit)) {
return N;
}
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 458222a2b6eccc..fa70ff5ed3780a 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -4484,7 +4484,7 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace A",
Style);
- Style.ColumnLimit = 40;
+ Style.ColumnLimit = 41;
verifyFormat("namespace aaaaaaaaaa {\n"
"namespace bbbbbbbbbb {\n"
"}} // namespace aaaaaaaaaa::bbbbbbbbbb",
@@ -4504,6 +4504,16 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace bbbbbb\n"
"} // namespace aaaaaa",
Style);
+
+ verifyFormat("namespace a { namespace b { namespace c {\n"
+ "}}} // namespace a::b::c",
+ Style);
+
+ verifyFormat("namespace a { namespace b {\n"
+ "namespace cc {\n"
+ "}}} // namespace a::b::cc",
+ Style);
+
Style.ColumnLimit = 80;
// Extra semicolon after 'inner' closing brace prevents merging
@@ -28318,6 +28328,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
auto BaseStyle = getLLVMStyle();
BaseStyle.AllowShortNamespacesOnASingleLine = true;
BaseStyle.FixNamespaceComments = false;
+ BaseStyle.CompactNamespaces = true;
auto Style = BaseStyle;
@@ -28332,8 +28343,9 @@ TEST_F(FormatTest, ShortNamespacesOption) {
Style);
// Trailing comments prevent merging.
- verifyFormat("namespace foo {\n"
- "namespace baz { class qux; } // comment\n"
+ verifyFormat("namespace foo { namespace baz {\n"
+ "class qux;\n"
+ "} // comment\n"
"}",
Style);
@@ -28348,12 +28360,23 @@ TEST_F(FormatTest, ShortNamespacesOption) {
// Nested namespaces.
verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
+
+ // Without CompactNamespaces, we won't merge consecutive namespace
+ // declarations
+ Style.CompactNamespaces = false;
+ verifyFormat("namespace foo {\n"
+ "namespace bar { class baz; }\n"
+ "}",
+ Style);
+
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }\n"
"namespace qux { class quux; }\n"
"}",
Style);
+ Style = BaseStyle;
+
// Varying inner content.
verifyFormat("namespace foo {\n"
"int f() { return 5; }\n"
@@ -28362,7 +28385,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
verifyFormat("namespace foo { template <T> struct bar; }", Style);
verifyFormat("namespace foo { constexpr int num = 42; }", Style);
- // Validate wrapping scenarios around the ColumnLimit.
+ // Validate nested namespace wrapping scenarios around the ColumnLimit.
Style.ColumnLimit = 64;
// Validate just under the ColumnLimit.
@@ -28371,22 +28394,24 @@ TEST_F(FormatTest, ShortNamespacesOption) {
Style);
// Validate just over the ColumnLimit.
- verifyFormat("namespace foo {\n"
- "namespace bar { namespace baz { class quux; } }\n"
- "}",
+ verifyFormat("namespace foo { namespace baar { namespace baaz {\n"
+ "class quux;\n"
+ "}}}",
Style);
- verifyFormat("namespace foo {\n"
- "namespace bar {\n"
- "namespace baz { namespace qux { class quux; } }\n"
- "}\n"
- "}",
- Style);
+ verifyFormat(
+ "namespace foo { namespace bar { namespace baz { namespace qux {\n"
+ "class quux;\n"
+ "}}}}",
+ Style);
// Validate that the ColumnLimit logic accounts for trailing content as well.
- verifyFormat("namespace foo {\n"
- "namespace bar { namespace baz { class qux; } }\n"
- "} // extra",
+ verifyFormat("namespace foo { namespace bar { class qux; } } // extra",
+ Style);
+
+ verifyFormat("namespace foo { namespace bar { namespace baz {\n"
+ "class qux;\n"
+ "}}} // extra",
Style);
// No ColumnLimit, allows long nested one-liners, but also leaves multi-line
@@ -28401,41 +28426,24 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- Style = BaseStyle;
- Style.CompactNamespaces = true;
verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
- // If we can't merge an outer nested namespaces, but can merge an inner
- // nested namespace, then CompactNamespaces will merge the outer namespace
- // first, preventing the merging of the inner namespace.
- verifyFormat("namespace foo { namespace baz {\n"
- "class qux;\n"
- "} // comment\n"
- "}",
- Style);
-
- // This option doesn't really work with FixNamespaceComments and nested
- // namespaces. Code should use the concatenated namespace syntax. e.g.
- // 'namespace foo::bar'.
+ // FIXME: Ideally AllowShortNamespacesOnASingleLine would disable the trailing
+ // namespace comment from 'FixNamespaceComments', as it's not really necessary
+ // in this scenario, but the two options work at very different layers of the
+ // formatter, so I'm not sure how to make them interact.
+ //
+ // As it stands, the trailing comment will be added and likely make the line
+ // too long to fit within the ColumnLimit, reducing the how likely the line
+ // will still fit on a single line. The recommendation for now is to use the
+ // concatenated namespace syntax instead. e.g. 'namespace foo::bar'
Style = BaseStyle;
Style.FixNamespaceComments = true;
verifyFormat(
- "namespace foo {\n"
- "namespace bar { namespace baz { class qux; } } // namespace bar\n"
- "} // namespace foo",
- "namespace foo { namespace bar { namespace baz { class qux; } } }",
- Style);
-
- // This option doesn't really make any sense with ShortNamespaceLines = 0.
- Style.ShortNamespaceLines = 0;
-
- verifyFormat(
- "namespace foo {\n"
- "namespace bar {\n"
- "namespace baz { class qux; } // namespace baz\n"
- "} // namespace bar\n"
- "} // namespace foo",
+ "namespace foo { namespace bar { namespace baz {\n"
+ "class qux;\n"
+ "}}} // namespace foo::bar::baz",
"namespace foo { namespace bar { namespace baz { class qux; } } }",
Style);
}
>From 5fcfbd22c359bc0b4e14806a253822a5aab5304d Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Tue, 22 Oct 2024 10:59:24 -0700
Subject: [PATCH 11/18] Remove ColumnLimit=0 tests
---
clang/unittests/Format/FormatTest.cpp | 14 --------------
1 file changed, 14 deletions(-)
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index fa70ff5ed3780a..4ed61f2ea2696d 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28414,20 +28414,6 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}}} // extra",
Style);
- // No ColumnLimit, allows long nested one-liners, but also leaves multi-line
- // instances alone.
- Style.ColumnLimit = 0;
- verifyFormat(
- "namespace foo { namespace bar { namespace baz { class qux; } } }",
- Style);
-
- verifyNoChange("namespace foo {\n"
- "namespace bar { namespace baz { class qux; } }\n"
- "}",
- Style);
-
- verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
-
// FIXME: Ideally AllowShortNamespacesOnASingleLine would disable the trailing
// namespace comment from 'FixNamespaceComments', as it's not really necessary
// in this scenario, but the two options work at very different layers of the
>From 780ac082df25aae09970a33e0cacf9b5e3c7a553 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Fri, 8 Nov 2024 15:14:18 -0800
Subject: [PATCH 12/18] Remove blank line in release notes
---
clang/docs/ReleaseNotes.rst | 1 -
1 file changed, 1 deletion(-)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1ab0154d71b99c..6309f724b930b6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1121,7 +1121,6 @@ clang-format
``Never``, and ``true`` to ``Always``.
- Adds ``RemoveEmptyLinesInUnwrappedLines`` option.
- Adds ``KeepFormFeed`` option and set it to ``true`` for ``GNU`` style.
-
- Adds ``AllowShortNamespacesOnASingleLine`` option.
libclang
>From 1c10f8a8ff0ce4d44718d9d81412fcd835519e4d Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Fri, 27 Dec 2024 21:13:57 -0800
Subject: [PATCH 13/18] PR review comments
---
clang/include/clang/Format/Format.h | 1 +
clang/lib/Format/UnwrappedLineFormatter.cpp | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 732057334578b6..c7c90ca43ea1df 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -5172,6 +5172,7 @@ struct FormatStyle {
R.AllowShortIfStatementsOnASingleLine &&
AllowShortLambdasOnASingleLine == R.AllowShortLambdasOnASingleLine &&
AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
+ AllowShortNamespacesOnASingleLine == R.AllowShortNamespacesOnASingleLine &&
AlwaysBreakBeforeMultilineStrings ==
R.AlwaysBreakBeforeMultilineStrings &&
AttributeMacros == R.AttributeMacros &&
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index d9fa2780034b45..90abb70e6cbcff 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -653,7 +653,7 @@ class LineJoiner {
assert(Limit >= I[1]->Last->TotalLength + 3);
const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
- if (!MergedLines)
+ if (MergedLines == 0)
return 0;
const auto N = MergedLines + 2;
// Check if there is even a line after the inner result.
>From 549d77883910d45f95927d9a071fc63184b528d9 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Fri, 27 Dec 2024 21:43:22 -0800
Subject: [PATCH 14/18] Clang-format previous commit
---
clang/include/clang/Format/Format.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index c7c90ca43ea1df..eefaabf9392fd7 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -5172,7 +5172,8 @@ struct FormatStyle {
R.AllowShortIfStatementsOnASingleLine &&
AllowShortLambdasOnASingleLine == R.AllowShortLambdasOnASingleLine &&
AllowShortLoopsOnASingleLine == R.AllowShortLoopsOnASingleLine &&
- AllowShortNamespacesOnASingleLine == R.AllowShortNamespacesOnASingleLine &&
+ AllowShortNamespacesOnASingleLine ==
+ R.AllowShortNamespacesOnASingleLine &&
AlwaysBreakBeforeMultilineStrings ==
R.AlwaysBreakBeforeMultilineStrings &&
AttributeMacros == R.AttributeMacros &&
>From df397a9409a2c6f812b05bd3169ff6c8cbeab427 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Sat, 28 Dec 2024 11:45:33 -0800
Subject: [PATCH 15/18] PR comment cleanup
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 90abb70e6cbcff..a8f28fa2162e59 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -365,13 +365,12 @@ class LineJoiner {
// FIXME: There are probably cases where we should use FirstNonComment
// instead of TheLine->First.
- if (TheLine->Last->is(tok::l_brace)) {
- if (Style.AllowShortNamespacesOnASingleLine &&
- TheLine->First->is(tok::kw_namespace)) {
- unsigned result = tryMergeNamespace(I, E, Limit);
- if (result > 0)
- return result;
- }
+ if (Style.AllowShortNamespacesOnASingleLine &&
+ TheLine->First->is(tok::kw_namespace) &&
+ TheLine->Last->is(tok::l_brace)) {
+ const auto result = tryMergeNamespace(I, E, Limit);
+ if (result > 0)
+ return result;
}
if (Style.CompactNamespaces) {
>From 9e50108853f9ab0be3568b1029bce510e70a2dac Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at microsoft.com>
Date: Sun, 29 Dec 2024 21:53:53 -0800
Subject: [PATCH 16/18] More PR comment cleanup
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 30 +++++++++++++--------
clang/unittests/Format/FormatTest.cpp | 10 +++----
2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index a8f28fa2162e59..cac94699b46eff 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -631,11 +631,19 @@ class LineJoiner {
unsigned Limit) {
if (Limit == 0)
return 0;
- if (I[1]->InPPDirective != (*I)->InPPDirective ||
- (I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+ assert(I[1]);
+ const auto &L1 = *I[1];
+ if (L1.InPPDirective != (*I)->InPPDirective ||
+ (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
return 0;
}
- if (I + 2 == E || I[2]->Type == LT_Invalid)
+
+ if (std::distance(I, E) <= 2)
+ return 0;
+
+ assert(I[2]);
+ const auto &L2 = *I[2];
+ if (L2.Type == LT_Invalid)
return 0;
Limit = limitConsideringMacros(I + 1, E, Limit);
@@ -645,13 +653,13 @@ class LineJoiner {
// Check if it's a namespace inside a namespace, and call recursively if so.
// '3' is the sizes of the whitespace and closing brace for " _inner_ }".
- if (I[1]->First->is(tok::kw_namespace)) {
- if (I[1]->Last->is(tok::comment) || !Style.CompactNamespaces)
+ if (L1.First->is(tok::kw_namespace)) {
+ if (L1.Last->is(tok::comment) || !Style.CompactNamespaces)
return 0;
- assert(Limit >= I[1]->Last->TotalLength + 3);
- const unsigned InnerLimit = Limit - I[1]->Last->TotalLength - 3;
- const unsigned MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
+ assert(Limit >= L1.Last->TotalLength + 3);
+ const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
+ const auto MergedLines = tryMergeNamespace(I + 1, E, InnerLimit);
if (MergedLines == 0)
return 0;
const auto N = MergedLines + 2;
@@ -672,11 +680,11 @@ class LineJoiner {
// line.
// The line which is in the namespace should end with semicolon.
- if (I[1]->Last->isNot(tok::semi))
+ if (L1.Last->isNot(tok::semi))
return 0;
// Last, check that the third line starts with a closing brace.
- if (I[2]->First->isNot(tok::r_brace) || I[2]->First->MustBreakBefore)
+ if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
return 0;
// If so, merge all three lines.
@@ -987,7 +995,7 @@ class LineJoiner {
SmallVectorImpl<AnnotatedLine *>::const_iterator E,
unsigned Limit) {
unsigned JoinedLength = 0;
- for (auto J = I + 1; J != E; ++J) {
+ for (const auto *J = I + 1; J != E; ++J) {
if ((*J)->First->MustBreakBefore)
return false;
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 4ed61f2ea2696d..8f534c2a1fa17d 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -4484,7 +4484,7 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace A",
Style);
- Style.ColumnLimit = 41;
+ Style.ColumnLimit = 40;
verifyFormat("namespace aaaaaaaaaa {\n"
"namespace bbbbbbbbbb {\n"
"}} // namespace aaaaaaaaaa::bbbbbbbbbb",
@@ -4505,13 +4505,9 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace aaaaaa",
Style);
- verifyFormat("namespace a { namespace b { namespace c {\n"
- "}}} // namespace a::b::c",
- Style);
-
verifyFormat("namespace a { namespace b {\n"
- "namespace cc {\n"
- "}}} // namespace a::b::cc",
+ "namespace c {\n"
+ "}}} // namespace a::b::c",
Style);
Style.ColumnLimit = 80;
>From 8380509572dc27212b6183b7c6df5284af77dbfa Mon Sep 17 00:00:00 2001
From: Owen Pan <owenpiano at gmail.com>
Date: Mon, 30 Dec 2024 00:59:03 -0800
Subject: [PATCH 17/18] Apply suggestions from code review
Final NFC cleanup
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 1 +
clang/unittests/Format/FormatTest.cpp | 16 ++++++----------
2 files changed, 7 insertions(+), 10 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cac94699b46eff..803c600cec44db 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -631,6 +631,7 @@ class LineJoiner {
unsigned Limit) {
if (Limit == 0)
return 0;
+
assert(I[1]);
const auto &L1 = *I[1];
if (L1.InPPDirective != (*I)->InPPDirective ||
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 8f534c2a1fa17d..2a8eb7b73620c1 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28321,12 +28321,10 @@ TEST_F(FormatTest, KeepFormFeed) {
}
TEST_F(FormatTest, ShortNamespacesOption) {
- auto BaseStyle = getLLVMStyle();
- BaseStyle.AllowShortNamespacesOnASingleLine = true;
- BaseStyle.FixNamespaceComments = false;
- BaseStyle.CompactNamespaces = true;
-
- auto Style = BaseStyle;
+ auto Style = getLLVMStyle();
+ Style.AllowShortNamespacesOnASingleLine = true;
+ Style.CompactNamespaces = true;
+ Style.FixNamespaceComments = false;
// Basic functionality.
verifyFormat("namespace foo { class bar; }", Style);
@@ -28358,7 +28356,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
verifyFormat("namespace foo { namespace bar { class baz; } }", Style);
// Without CompactNamespaces, we won't merge consecutive namespace
- // declarations
+ // declarations.
Style.CompactNamespaces = false;
verifyFormat("namespace foo {\n"
"namespace bar { class baz; }\n"
@@ -28371,7 +28369,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}",
Style);
- Style = BaseStyle;
+ Style.CompactNamespaces = true;
// Varying inner content.
verifyFormat("namespace foo {\n"
@@ -28419,9 +28417,7 @@ TEST_F(FormatTest, ShortNamespacesOption) {
// too long to fit within the ColumnLimit, reducing the how likely the line
// will still fit on a single line. The recommendation for now is to use the
// concatenated namespace syntax instead. e.g. 'namespace foo::bar'
- Style = BaseStyle;
Style.FixNamespaceComments = true;
-
verifyFormat(
"namespace foo { namespace bar { namespace baz {\n"
"class qux;\n"
>From 280e2a46e96e0ceaf40fad0f3afbda0fb0169815 Mon Sep 17 00:00:00 2001
From: Owen Pan <owenpiano at gmail.com>
Date: Mon, 30 Dec 2024 01:01:06 -0800
Subject: [PATCH 18/18] Update FormatTest.cpp
---
clang/unittests/Format/FormatTest.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 2a8eb7b73620c1..22b6f7e1b62e2e 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -4476,6 +4476,7 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"int k; } // namespace out",
Style);
+ Style.ColumnLimit = 41;
verifyFormat("namespace A { namespace B { namespace C {\n"
"}}} // namespace A::B::C",
"namespace A { namespace B {\n"
More information about the cfe-commits
mailing list