This is an archive of the discontinued LLVM Phabricator instance.

Differential D158505

[clang-format] Fix weird handling of AfterColon
ClosedPublic

Authored by HazardyKnusperkeks on Aug 22 2023, 4:20 AM.

Details

Reviewers
rymiel
owenpan
MyDeveloperDay
Commits
rG6e77ba6c1e4d: [clang-format] Fix weird handling of AfterColon
Summary

This fixes https://github.com/llvm/llvm-project/issues/64895.

Diff Detail

Unit TestsFailed

TimeTest
90,660 msclang CI - Running libc++ test suite with Clang Modules > llvm-libc++-shared-cfg-in.llvm-libc++-shared-cfg-in::/var/lib/buildkite-agent/builds/llvm-project/build/generic-modules/test/libcxx/modules_include.gen.py/__std_clang_module.compile.pass.mm

Event Timeline

HazardyKnusperkeks created this revision.Aug 22 2023, 4:20 AM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptAug 22 2023, 4:20 AM
Herald added reviewers: rymiel, owenpan, MyDeveloperDay. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
HazardyKnusperkeks requested review of this revision.Aug 22 2023, 4:20 AM
HazardyKnusperkeks added inline comments.
clang/unittests/Format/FormatTest.cpp
7995

I know we usually don't change existing tests. But I sincerely think this a bug. From the documentation I don't see any reason why there should be a line break.

Harbormaster completed remote builds in B254057: Diff 552310.Aug 22 2023, 5:01 AM
owenpan accepted this revision.Aug 23 2023, 12:50 AM
owenpan added inline comments.
clang/unittests/Format/FormatTest.cpp
7954

loooooooooooooooooooong?

7994–7997

Shorten the ctor name so that we don't have to split the line.

This revision is now accepted and ready to land.Aug 23 2023, 12:50 AM
HazardyKnusperkeks marked 2 inline comments as done.Aug 23 2023, 3:45 AM

Will be fixed on commit.

This revision was landed with ongoing or failed builds.Aug 23 2023, 11:34 PM
Closed by commit rG6e77ba6c1e4d: [clang-format] Fix weird handling of AfterColon (authored by HazardyKnusperkeks). · Explain Why
This revision was automatically updated to reflect the committed changes.
HazardyKnusperkeks added a commit: rG6e77ba6c1e4d: [clang-format] Fix weird handling of AfterColon.

Revision Contents

PathSize
clang/
lib/
Format/
2 lines
unittests/
Format/
9 lines

Diff 552310

clang/lib/Format/ContinuationIndenter.cpp

Show First 20 Lines • Show All 1,445 Lines • ▼ Show 20 Linesunsigned ContinuationIndenter::moveStateToNextToken(LineState &State,
} }
if (Current.is(TT_CtorInitializerColon) && if (Current.is(TT_CtorInitializerColon) &&
Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon) { Style.BreakConstructorInitializers == FormatStyle::BCIS_AfterColon) {
CurrentState.Indent = CurrentState.Indent =
State.FirstIndent + Style.ConstructorInitializerIndentWidth; State.FirstIndent + Style.ConstructorInitializerIndentWidth;
CurrentState.NestedBlockIndent = CurrentState.Indent; CurrentState.NestedBlockIndent = CurrentState.Indent;
if (Style.PackConstructorInitializers > FormatStyle::PCIS_BinPack) if (Style.PackConstructorInitializers > FormatStyle::PCIS_BinPack)
CurrentState.AvoidBinPacking = true; CurrentState.AvoidBinPacking = true;
else
CurrentState.BreakBeforeParameter = false;
} }
if (Current.is(TT_InheritanceColon)) { if (Current.is(TT_InheritanceColon)) {
CurrentState.Indent = CurrentState.Indent =
State.FirstIndent + Style.ConstructorInitializerIndentWidth; State.FirstIndent + Style.ConstructorInitializerIndentWidth;
} }
if (Current.isOneOf(TT_BinaryOperator, TT_ConditionalExpr) && Newline) if (Current.isOneOf(TT_BinaryOperator, TT_ConditionalExpr) && Newline)
CurrentState.NestedBlockIndent = State.Column + Current.ColumnWidth + 1; CurrentState.NestedBlockIndent = State.Column + Current.ColumnWidth + 1;
if (Current.isOneOf(TT_LambdaLSquare, TT_LambdaArrow)) if (Current.isOneOf(TT_LambdaLSquare, TT_LambdaArrow))
▲ Show 20 Lines • Show All 1,184 Lines • Show Last 20 Lines

clang/unittests/Format/FormatTest.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 7,944 Lines • ▼ Show 20 Lines verifyFormat("Constructor() :\n"
getStyleWithColumns(Style, 44)); getStyleWithColumns(Style, 44));
verifyFormat("Constructor() :\n" verifyFormat("Constructor() :\n"
" Initializer(FitsOnTheLine) {}", " Initializer(FitsOnTheLine) {}",
getStyleWithColumns(Style, 43)); getStyleWithColumns(Style, 43));
verifyFormat("template <typename T>\n" verifyFormat("template <typename T>\n"
"Constructor() : Initializer(FitsOnTheLine) {}", "Constructor() : Initializer(FitsOnTheLine) {}",
getStyleWithColumns(Style, 50)); getStyleWithColumns(Style, 50));
verifyFormat(
"Class::Class(int some, int arguments, int loooooooooooooonoooooog,\n"
owenpanUnsubmitted
Done
ReplyInline Actions

loooooooooooooooooooong?

owenpan: `loooooooooooooooooooong`?
" int mooooooooooooore) noexcept :\n"
" Super{some, arguments}, Member{5}, Member2{2} {}",
Style);
Style.PackConstructorInitializers = FormatStyle::PCIS_NextLine; Style.PackConstructorInitializers = FormatStyle::PCIS_NextLine;
verifyFormat( verifyFormat(
"SomeClass::Constructor() :\n" "SomeClass::Constructor() :\n"
" aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}", " aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
Style); Style);
verifyFormat( verifyFormat(
"SomeClass::Constructor() : // NOLINT\n" "SomeClass::Constructor() : // NOLINT\n"
" aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}", " aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
Show All 20 Lines verifyFormat(
" aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n" " aaaaaaaaaaaaa(aaaaaaaaaaaaaa), aaaaaaaaaaaaa(aaaaaaaaaaaaaa),\n"
" aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}", " aaaaaaaaaaaaa(aaaaaaaaaaaaaa) {}",
Style); Style);
verifyFormat( verifyFormat(
"SomeClass::Constructor() :\n" "SomeClass::Constructor() :\n"
" aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n" " aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
" aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}", " aaaaaaaaaaaaaaa(aaaaaaaaaaaa) {}",
Style); Style);
verifyFormat("Constructor(aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n" verifyFormat("Constructor(aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
" aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) :\n" " aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) : "
HazardyKnusperkeksAuthorUnsubmitted
Done
ReplyInline Actions

I know we usually don't change existing tests. But I sincerely think this a bug. From the documentation I don't see any reason why there should be a line break.

HazardyKnusperkeks: I know we usually don't change existing tests. But I sincerely think this a bug. From the…
" aaaaaaaaaa(aaaaaa) {}", "aaaaaaaaaa(aaaaaa) {}",
Style); Style);
owenpanUnsubmitted
Done
ReplyInline Actions
Style);
- verifyFormat("Constructor(aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
- " aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) : "
- "aaaaaaaaaa(aaaaaa) {}",
- Style);
+ verifyFormat(
+ "Ctor(aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
+ " aaaaaa aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa) : aaaaaaaaaa(aaaaaa) {}",
+ Style);
verifyFormat("Constructor() :\n"

Shorten the ctor name so that we don't have to split the line.

owenpan: Shorten the ctor name so that we don't have to split the line.
verifyFormat("Constructor() :\n" verifyFormat("Constructor() :\n"
" aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n" " aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
" aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n" " aaaaaaaaaaaaaaaaaaaaaaaa(aaaaaaaaaaaaaaaaaaaaaaaaaaa,\n"
" aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n" " aaaaaaaaaaaaaaaaaaaaaaaaaaa),\n"
" aaaaaaaaaaaaaaaaaaaaaaa() {}", " aaaaaaaaaaaaaaaaaaaaaaa() {}",
Style); Style);
▲ Show 20 Lines • Show All 18,364 Lines • Show Last 20 Lines