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