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 1/3] 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 2/3] 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 3/3] 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( _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
