[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)
Owen Pan via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 13 18:47:08 PST 2025
https://github.com/owenca updated https://github.com/llvm/llvm-project/pull/123010
>From 9d60d4980f1edbdd4cd4a9499f69e9d225717238 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at gmail.com>
Date: Tue, 14 Jan 2025 20:44:10 -0800
Subject: [PATCH 1/6] Support BraceWrapping.AfterNamespace with
AllowShortNamespacesOnASingleLine
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 awkard.
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, rathern than the
AnnotatedLines, but I'm not seeing a straightforward way to do that.
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 45 +++++++++++++--------
clang/unittests/Format/FormatTest.cpp | 20 +++++++++
2 files changed, 49 insertions(+), 16 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index cee84fb1191ab..787136a26b378 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 4d48bcacddead..bf008e61490f5 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) {
>From e4756d8709366011a8102720c61bb22a794adb58 Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at gmail.com>
Date: Tue, 21 Jan 2025 14:00:58 -0800
Subject: [PATCH 2/6] Add a few more tests
---
clang/unittests/Format/FormatTest.cpp | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index bf008e61490f5..5b643055e8f66 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28441,6 +28441,11 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"class bar;\n"
"}",
Style);
+ verifyFormat("namespace foo { class bar; }",
+ "namespace foo {\n"
+ "class bar;\n"
+ "}",
+ Style);
verifyFormat("namespace foo\n"
"{\n"
@@ -28450,6 +28455,11 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"}\n"
"}\n",
Style);
+ verifyFormat("namespace foo // comment\n"
+ "{\n"
+ "class baz;\n"
+ "}\n",
+ Style);
}
TEST_F(FormatTest, WrapNamespaceBodyWithEmptyLinesNever) {
>From 47af7c2ce32a9dc04da719e0c9f7e8a16faac87e Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at gmail.com>
Date: Mon, 27 Jan 2025 09:31:28 -0800
Subject: [PATCH 3/6] Fix signed/unsigned comparison warning
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 787136a26b378..90450bd17c660 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -638,7 +638,7 @@ class LineJoiner {
// 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 OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
const auto BraceOpenLine = I + OpeningBraceLineOffset;
if (std::distance(BraceOpenLine, E) <= 2)
>From 0d5e911d65a75a7fa0311d4c27182d07ed5dea5e Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at gmail.com>
Date: Tue, 11 Feb 2025 15:20:58 -0800
Subject: [PATCH 4/6] Address review comments
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 28 ++++++++++-----------
clang/unittests/Format/FormatTest.cpp | 4 +--
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 90450bd17c660..18c2577de14a8 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -366,13 +366,8 @@ class LineJoiner {
// instead of TheLine->First.
if (Style.AllowShortNamespacesOnASingleLine &&
- TheLine->First->is(tok::kw_namespace) &&
- ((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);
+ TheLine->First->is(tok::kw_namespace)) {
+ const auto result = tryMergeNamespace(I, E, Limit);
if (result > 0)
return result;
}
@@ -632,14 +627,18 @@ class LineJoiner {
unsigned tryMergeNamespace(ArrayRef<AnnotatedLine *>::const_iterator I,
ArrayRef<AnnotatedLine *>::const_iterator E,
- unsigned Limit, bool OpenBraceWrapped) {
+ unsigned Limit) {
if (Limit == 0)
return 0;
// 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 auto OpeningBraceLineOffset = OpenBraceWrapped ? 1 : 0;
- const auto BraceOpenLine = I + OpeningBraceLineOffset;
+ const bool OpenBraceWrapped = Style.BraceWrapping.AfterNamespace;
+ const auto BraceOpenLine = I + OpenBraceWrapped;
+
+ assert(*BraceOpenLine);
+ if ((*BraceOpenLine)->Last->isNot(tok::l_brace))
+ return 0;
if (std::distance(BraceOpenLine, E) <= 2)
return 0;
@@ -661,7 +660,8 @@ class LineJoiner {
Limit = limitConsideringMacros(I + 1, E, Limit);
- if (!nextNLinesFitInto(I, I + OpeningBraceLineOffset + 2, Limit))
+ const auto LinesToBeMerged = OpenBraceWrapped + 2;
+ if (!nextNLinesFitInto(I, I + LinesToBeMerged, Limit))
return 0;
// Check if it's a namespace inside a namespace, and call recursively if so.
@@ -673,10 +673,10 @@ class LineJoiner {
assert(Limit >= L1.Last->TotalLength + 3);
const auto InnerLimit = Limit - L1.Last->TotalLength - 3;
const auto MergedLines =
- tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit, OpenBraceWrapped);
+ tryMergeNamespace(BraceOpenLine + 1, E, InnerLimit);
if (MergedLines == 0)
return 0;
- const auto N = MergedLines + 2 + OpeningBraceLineOffset;
+ const auto N = MergedLines + LinesToBeMerged;
// Check if there is even a line after the inner result.
if (std::distance(I, E) <= N)
return 0;
@@ -702,7 +702,7 @@ class LineJoiner {
return 0;
// If so, merge all lines.
- return 2 + OpeningBraceLineOffset;
+ return LinesToBeMerged;
}
unsigned
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 5b643055e8f66..7847e42d773ef 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28453,12 +28453,12 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"{ // comment\n"
"class baz;\n"
"}\n"
- "}\n",
+ "}",
Style);
verifyFormat("namespace foo // comment\n"
"{\n"
"class baz;\n"
- "}\n",
+ "}",
Style);
}
>From d74a8b2046beefa85d64c727c9c4bbccf7f64a7d Mon Sep 17 00:00:00 2001
From: Galen Elias <gelias at gmail.com>
Date: Thu, 13 Feb 2025 12:52:23 -0800
Subject: [PATCH 5/6] Address more review comments
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 9 +++++----
clang/unittests/Format/FormatTest.cpp | 1 -
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 18c2577de14a8..652a7eb8f301d 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -637,13 +637,13 @@ class LineJoiner {
const auto BraceOpenLine = I + OpenBraceWrapped;
assert(*BraceOpenLine);
- if ((*BraceOpenLine)->Last->isNot(tok::l_brace))
+ if (BraceOpenLine[0]->Last->isNot(TT_NamespaceLBrace))
return 0;
if (std::distance(BraceOpenLine, E) <= 2)
return 0;
- if (BraceOpenLine[0]->Last->is(TT_LineComment))
+ if (BraceOpenLine[0]->Last->is(tok::comment))
return 0;
assert(BraceOpenLine[1]);
@@ -682,7 +682,8 @@ 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(TT_NamespaceRBrace) &&
+ !I[N]->First->MustBreakBefore &&
BraceOpenLine[MergedLines + 1]->Last->isNot(tok::comment) &&
nextNLinesFitInto(I, I + N + 1, Limit)) {
return N;
@@ -698,7 +699,7 @@ class LineJoiner {
return 0;
// Last, check that the third line starts with a closing brace.
- if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
+ if (L2.First->isNot(TT_NamespaceRBrace) || L2.First->MustBreakBefore)
return 0;
// If so, merge all lines.
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 7847e42d773ef..c7bfbcf3e5d35 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -28446,7 +28446,6 @@ TEST_F(FormatTest, ShortNamespacesOption) {
"class bar;\n"
"}",
Style);
-
verifyFormat("namespace foo\n"
"{\n"
"namespace bar\n"
>From 1415aaecb73205547e5a00af93d83ab2a1bcbeec Mon Sep 17 00:00:00 2001
From: Owen Pan <owenpiano at gmail.com>
Date: Thu, 13 Feb 2025 18:46:58 -0800
Subject: [PATCH 6/6] Update UnwrappedLineFormatter.cpp
---
clang/lib/Format/UnwrappedLineFormatter.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index 652a7eb8f301d..d4d72f1174c29 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -634,7 +634,7 @@ class LineJoiner {
// 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 bool OpenBraceWrapped = Style.BraceWrapping.AfterNamespace;
- const auto BraceOpenLine = I + OpenBraceWrapped;
+ const auto *BraceOpenLine = I + OpenBraceWrapped;
assert(*BraceOpenLine);
if (BraceOpenLine[0]->Last->isNot(TT_NamespaceLBrace))
More information about the cfe-commits
mailing list