[clang] [clang-format] Support BraceWrapping.AfterNamespace with AllowShortNamespacesOnASingleLine (PR #123010)
Galen Elias via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 21 14:02:47 PST 2025
https://github.com/galenelias 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/2] 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 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) {
>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/2] 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 bf008e61490f57..5b643055e8f665 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) {
More information about the cfe-commits
mailing list