[clang-format] Add AllowShortNamespacesOnASingleLine option
#105597
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-format Author: Galen Elias (galenelias) ChangesThis addresses: #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: 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. Full diff: https://github.com/llvm/llvm-project/pull/105597.diff 4 Files Affected:
diff --git a/clang/include/clang/Format/Format.h b/clang/include/clang/Format/Format.h
index fea5d2e17a0e28..9d72b82549afc2 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 7fd42e46e0ccb7..1272aed7e9b042 100644
--- a/clang/lib/Format/Format.cpp
+++ b/clang/lib/Format/Format.cpp
@@ -941,6 +941,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",
@@ -1445,6 +1447,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 2a754a29e81e73..3343174d25446c 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -27659,6 +27659,118 @@ TEST_F(FormatTest, SpaceBetweenKeywordAndLiteral) {
verifyFormat("return sizeof \"5\";");
}
+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
|
galenelias
force-pushed
the
user/gelias/issue101363_AllowShortNamespacesOnASingleLine
branch
from
1ed2dbe to
4118b7d
Compare
There was a problem hiding this comment.
The following are missing:
- Run
dump_format_style.py. - Add a
ConfigParseTestfor the new option.
How does the new option interact with CompactNamespaces? For example:
AllowShortNamespacesOnASingleLine: trueandCompactNamespaces: false
namespace a {
namespace b { class c; }
} // namespace a
- Both
true
namespace a { namespace b { class c; } }
Aah, I hadn't considered the interaction of this AllowShortNamespacesOnASingleLine and CompactNamespaces, as AllowShortNamespacesOnASingleLine effectively forces CompactNamespaces behavior when the block can all fit on a single line. However, it does appear that they were in conflict where the CompactNamespaces code was running first and then short circuiting out of the AllowShortNamespacesOnASingleLine logic. Moved the logic around a bit, as AllowShortNamespacesOnASingleLine is mostly a superset of the CompactNamespaces logic. There are still some ambiguous situations around nested namespaces with a single statement inside that could be merged with both these two rules. I am just letting those get merged with CompactNamespaces first if the full expression can't be pulled onto a single line. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Ping |
IMO
|
As an active user of ColumnLimit 0, I disagree. At least for my team we use ColumnLimit of 0 to allow for more customization and variability of the column limit. Treating it like ColumnLimit: Infinity causes huge constructs to be pulled into a single line for some of these 'AllowShort' options, which we actively don't want. Making somewhat arbitrary and contextual decisions about how to do the breaking is a 'feature' in my opinion - although I'm not sure if that's truly the intent and vision behind ColumnLimit 0. My thought here is that this feature should function similarly to how CompactNamespaces interacts with ColumnLimit: 0, which is basically that it allows arbitrary line breaking. So you could have a 1000 character line, or you could break it after every namespace. Forcing it to always pull single statements no matter how long onto a single line would be undesirable in my opinion. I'm definitely open to changing things here, and being consistent if there is some other vision of how features are supposed to interact with ColumnLimit 0, but this has been my mental model so far, and that seems to be the natural behavior that I've observed, so validating that functionality in the tests seems reasonable to me. |
Below is the documentation on
The term statements above can be loosely interpreted as unwrapped lines. Since all the
IMO a better way to have arbitrary line breakings is to add a
I don't think we want to add any tests whose formats are questionable or still under discussion. WDYT @mydeveloperday @HazardyKnusperkeks @rymiel ? |
Ok, that's fair. I have removed these tests. Thanks for the detailed response and context. |
|
Ping |
owenca
changed the title
AllowShortNamespacesOnASingleLine option
This addresses: llvm#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.
- 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
galenelias
force-pushed
the
user/gelias/issue101363_AllowShortNamespacesOnASingleLine
branch
from
1733719 to
549d778
Compare
There was a problem hiding this comment.
Also, additional cleanup of tryMergeNamespace():
if (Limit == 0)
return 0;
- if (I[1]->InPPDirective != (*I)->InPPDirective ||
- (I[1]->InPPDirective && I[1]->First->HasUnescapedNewline)) {
+
+ assert(I[1]);
+ const auto &L1 = *I[1];
+ if (L1.InPPDirective != (*I)->InPPDirective ||
+ (L1.InPPDirective && L1.First->HasUnescapedNewline)) {
return 0;
}
- if (I + 2 == E || I[2]->Type == LT_Invalid)
+
+ 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);
@@ -645,13 +654,13 @@ private:
// 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) || !Style.CompactNamespaces)
+ if (L1.First->is(tok::kw_namespace)) {
+ if (L1.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);
+ 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;
@@ -672,11 +681,11 @@ private:
// line.
// The line which is in the namespace should end with semicolon.
- if (I[1]->Last->isNot(tok::semi))
+ if (L1.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)
+ if (L2.First->isNot(tok::r_brace) || L2.First->MustBreakBefore)
return 0;Use more descriptive names instead of L1 and L2 if you like.
Final NFC cleanup
|
@galenelias Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
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:
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
CompactNamespacescode. For nested namespaces with 3 or more namespaces, it was incorrectly compacting lines which were 1 or two spaces over theColumnLimit, leading to incorrect formatting results.