[clang] 486ec4b - [clang-format] Add `AllowShortNamespacesOnASingleLine` option (#105597)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 30 01:28:08 PST 2024
Author: Galen Elias
Date: 2024-12-30T01:28:03-08:00
New Revision: 486ec4bd7466cda444a7da6386a1bbb2db89a33f
URL: https://github.com/llvm/llvm-project/commit/486ec4bd7466cda444a7da6386a1bbb2db89a33f
DIFF: https://github.com/llvm/llvm-project/commit/486ec4bd7466cda444a7da6386a1bbb2db89a33f.diff
LOG: [clang-format] Add `AllowShortNamespacesOnASingleLine` option (#105597)
This fixes #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.
Also, fix an 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.
Added:
Modified:
clang/docs/ClangFormatStyleOptions.rst
clang/docs/ReleaseNotes.rst
clang/include/clang/Format/Format.h
clang/lib/Format/Format.cpp
clang/lib/Format/UnwrappedLineFormatter.cpp
clang/unittests/Format/ConfigParseTest.cpp
clang/unittests/Format/FormatTest.cpp
Removed:
################################################################################
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 4be448171699ca..c175436a2817a9 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -2088,6 +2088,11 @@ 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 line.
+
.. _AlwaysBreakAfterDefinitionReturnType:
**AlwaysBreakAfterDefinitionReturnType** (``DefinitionReturnTypeBreakingStyle``) :versionbadge:`clang-format 3.7` :ref:`¶ <AlwaysBreakAfterDefinitionReturnType>`
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 210ccc16eeb4fc..b7da12bcf65818 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -1123,6 +1123,7 @@ 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
--------
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index 6383934afa2c40..eefaabf9392fd7 100644
--- a/clang/include/clang/Format/Format.h
+++ b/clang/include/clang/Format/Format.h
@@ -988,6 +988,10 @@ struct FormatStyle {
/// \version 3.7
bool AllowShortLoopsOnASingleLine;
+ /// If ``true``, ``namespace a { class b; }`` can be put on a single line.
+ /// \version 20
+ 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 {
@@ -5168,6 +5172,8 @@ 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/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..803c600cec44db 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 (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) {
if (const auto *NSToken = TheLine->First->getNamespaceToken()) {
int J = 1;
@@ -373,7 +382,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.
@@ -420,6 +429,7 @@ class LineJoiner {
TheLine->First != LastNonComment) {
return MergeShortFunctions ? tryMergeSimpleBlock(I, E, Limit) : 0;
}
+
// 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 +626,72 @@ class LineJoiner {
return 1;
}
+ unsigned tryMergeNamespace(SmallVectorImpl<AnnotatedLine *>::const_iterator I,
+ SmallVectorImpl<AnnotatedLine *>::const_iterator E,
+ unsigned Limit) {
+ if (Limit == 0)
+ return 0;
+
+ assert(I[1]);
+ const auto &L1 = *I[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];
+ if (L2.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 (L1.First->is(tok::kw_namespace)) {
+ if (L1.Last->is(tok::comment) || !Style.CompactNamespaces)
+ return 0;
+
+ 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;
+ // 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) &&
+ nextNLinesFitInto(I, I + N + 1, Limit)) {
+ return N;
+ }
+ 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 (L1.Last->isNot(tok::semi))
+ return 0;
+
+ // Last, check that the third line starts with a closing brace.
+ if (L2.First->isNot(tok::r_brace) || L2.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 +992,21 @@ 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 (const auto *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/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);
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index 47465a18e9a41e..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"
@@ -4504,6 +4505,12 @@ TEST_F(FormatTest, FormatsCompactNamespaces) {
"} // namespace bbbbbb\n"
"} // namespace aaaaaa",
Style);
+
+ verifyFormat("namespace a { namespace b {\n"
+ "namespace c {\n"
+ "}}} // namespace a::b::c",
+ Style);
+
Style.ColumnLimit = 80;
// Extra semicolon after 'inner' closing brace prevents merging
@@ -28314,6 +28321,112 @@ TEST_F(FormatTest, KeepFormFeed) {
Style);
}
+TEST_F(FormatTest, ShortNamespacesOption) {
+ auto Style = getLLVMStyle();
+ Style.AllowShortNamespacesOnASingleLine = true;
+ Style.CompactNamespaces = 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 { namespace baz {\n"
+ "class qux;\n"
+ "} // comment\n"
+ "}",
+ Style);
+
+ // Make sure code doesn't walk too 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);
+
+ // 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.CompactNamespaces = true;
+
+ // 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 nested namespace 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 { namespace baar { namespace baaz {\n"
+ "class quux;\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 { namespace bar { class qux; } } // extra",
+ Style);
+
+ verifyFormat("namespace foo { namespace bar { namespace baz {\n"
+ "class qux;\n"
+ "}}} // extra",
+ 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
diff erent 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.FixNamespaceComments = true;
+ verifyFormat(
+ "namespace foo { namespace bar { namespace baz {\n"
+ "class qux;\n"
+ "}}} // namespace foo::bar::baz",
+ "namespace foo { namespace bar { namespace baz { class qux; } } }",
+ Style);
+}
+
} // namespace
} // namespace test
} // namespace format
More information about the cfe-commits
mailing list