https://github.com/galenelias updated https://github.com/llvm/llvm-project/pull/105597
>From 4118b7dde9adbee7b6aaf5d094d34cb6b64f6c77 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Wed, 21 Aug 2024 16:33:42 -0700 Subject: [PATCH 01/10] 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 2af1d4065c3cc1..c96fc4d1d9557b 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -972,6 +972,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 97fac41cdd3008..495b727e609df6 100644 --- a/clang/lib/Format/Format.cpp +++ b/clang/lib/Format/Format.cpp @@ -952,6 +952,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", @@ -1457,6 +1459,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 794ccab3704534..468cb6b98dd745 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -27981,6 +27981,118 @@ TEST_F(FormatTest, BreakBinaryOperations) { 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 86534c3ba6a82e2782a6bf96350e1e71fa633f63 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Thu, 22 Aug 2024 09:16:32 -0700 Subject: [PATCH 02/10] 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 468cb6b98dd745..0fb1ed2d47e31d 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -27982,106 +27982,106 @@ TEST_F(FormatTest, BreakBinaryOperations) { } 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" @@ -28090,7 +28090,7 @@ TEST_F(FormatTest, ShortNamespacesOption) { "} // namespace bar\n" "} // namespace foo", "namespace foo { namespace bar { namespace baz { class qux; } } }", - style); + Style); } } // namespace >From 2159b8c222b9e9c8e2fdf60211a171e0a58b58b1 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Wed, 28 Aug 2024 10:42:05 -0700 Subject: [PATCH 03/10] 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 0fb1ed2d47e31d..e03e0a871c8d19 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -27986,7 +27986,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); @@ -27996,13 +27996,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;", @@ -28011,7 +28011,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" @@ -28019,7 +28019,7 @@ TEST_F(FormatTest, ShortNamespacesOption) { "}", Style); - // Varying inner content + // Varying inner content. verifyFormat("namespace foo {\n" "int f() { return 5; }\n" "}", @@ -28027,15 +28027,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" "}", @@ -28048,14 +28048,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; } } } }", @@ -28070,7 +28070,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( @@ -28080,7 +28080,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 07ec70d700bd061c5ccb7803d22e96a887734895 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Thu, 5 Sep 2024 07:41:49 -0700 Subject: [PATCH 04/10] 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 c96fc4d1d9557b..e1d10e64c92fe0 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -974,7 +974,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 b5f727b25352823e40d1c07723e5f0312c4c53f9 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Tue, 10 Sep 2024 08:30:37 -0700 Subject: [PATCH 05/10] 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 c79a635d86a6ef..53c3d26fe1d96a 100644 --- a/clang/docs/ClangFormatStyleOptions.rst +++ b/clang/docs/ClangFormatStyleOptions.rst @@ -1976,6 +1976,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 986d2d54cacf9fed5b2706ea180dfbdc34153a35 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Tue, 10 Sep 2024 08:31:06 -0700 Subject: [PATCH 06/10] 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 2ee0df99353ff5..37edaa5fd9c128 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(BinPackParameters); CHECK_PARSE_BOOL(BreakAdjacentStringLiterals); >From 40a85d1d0730e684cabefffb4a4356ea104ccfda Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Tue, 10 Sep 2024 08:31:21 -0700 Subject: [PATCH 07/10] 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 e03e0a871c8d19..0dd38b8b3d4692 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -27982,9 +27982,11 @@ TEST_F(FormatTest, BreakBinaryOperations) { } 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); @@ -28002,7 +28004,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;", @@ -28015,7 +28017,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); @@ -28057,20 +28059,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 ad1fdbb0131b10acf213b7fe49b0421856eadbc8 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Wed, 25 Sep 2024 16:20:46 -0700 Subject: [PATCH 08/10] 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 e1d10e64c92fe0..f09cfc90f7a4c1 100644 --- a/clang/include/clang/Format/Format.h +++ b/clang/include/clang/Format/Format.h @@ -972,8 +972,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 0dd38b8b3d4692..434f0a0dfb6079 100644 --- a/clang/unittests/Format/FormatTest.cpp +++ b/clang/unittests/Format/FormatTest.cpp @@ -28074,7 +28074,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 b8a21f7843f496e697abc69f8e3cfae90a995d2d Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Thu, 26 Sep 2024 08:15:02 -0700 Subject: [PATCH 09/10] 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 e735f46f15ce62d7c63a9ad287025409208b8483 Mon Sep 17 00:00:00 2001 From: Galen Elias <gel...@microsoft.com> Date: Tue, 15 Oct 2024 14:59:50 -0700 Subject: [PATCH 10/10] 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 53c3d26fe1d96a..317fc99b498f8f 100644 --- a/clang/docs/ClangFormatStyleOptions.rst +++ b/clang/docs/ClangFormatStyleOptions.rst @@ -1979,8 +1979,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 5c156a9c073a9c..f139a196cf4941 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -410,6 +410,8 @@ clang-format - Adds ``BreakBinaryOperations`` option. +- 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 434f0a0dfb6079..14955342de8270 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 @@ -27985,6 +27995,7 @@ TEST_F(FormatTest, ShortNamespacesOption) { auto BaseStyle = getLLVMStyle(); BaseStyle.AllowShortNamespacesOnASingleLine = true; BaseStyle.FixNamespaceComments = false; + BaseStyle.CompactNamespaces = true; auto Style = BaseStyle; @@ -27999,8 +28010,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); @@ -28015,12 +28027,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" @@ -28029,7 +28052,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. @@ -28038,22 +28061,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 @@ -28068,41 +28093,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); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
