This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Ignore first token when finding MustBreak
ClosedPublic

Authored by rymiel on May 15 2023, 2:10 PM.

Details

Summary

When in ColumnLimit 0, the formatter looks for MustBreakBefore in the
line in order to check if a line is allowed to be merged onto one line.

However, since MustBreakBefore is really a property of the gap between
the token and the one previously, I belive the check is erroneous in
checking all the tokens in a line, since whether the previous line ended
with a forced line break should have no effect on whether the current
line is allowed to merge with the next one.

This patch changes the check to skip the first token in
LineJoiner.containsMustBreak.

This patch also changes a test, which is not ideal, but I believe the
test also suffered from this bug. The test case in question sets
AllowShortFunctionsOnASingleLine to "Empty", but the empty function in
said test case isn't merged to a single line, because of the very same
bug this patch fixes.

Fixes https://github.com/llvm/llvm-project/issues/62721

Diff Detail

Event Timeline

rymiel created this revision.May 15 2023, 2:10 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 15 2023, 2:10 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
rymiel requested review of this revision.May 15 2023, 2:10 PM
owenpan added inline comments.May 15 2023, 7:02 PM
clang/lib/Format/UnwrappedLineFormatter.cpp
891–895

We can start from the second token.

rymiel updated this revision to Diff 522453.May 15 2023, 11:34 PM
rymiel marked an inline comment as done.

Start iteration from second token

clang/lib/Format/UnwrappedLineFormatter.cpp
891–895

Thanks! I'm really glad to have reviewers to catch times where I wasn't thinking straight

owenpan accepted this revision.May 15 2023, 11:38 PM
This revision is now accepted and ready to land.May 15 2023, 11:38 PM
MyDeveloperDay accepted this revision.May 16 2023, 2:26 AM
clang/unittests/Format/FormatTest.cpp
20137

This doesn't seem right.

rymiel added inline comments.May 16 2023, 12:56 PM
clang/unittests/Format/FormatTest.cpp
20137

WhitesmithsBraceStyle.AllowShortFunctionsOnASingleLine = FormatStyle::SFS_Empty; is set above

20137

(this is mentioned in the summary)

HazardyKnusperkeks added inline comments.
clang/unittests/Format/FormatTest.cpp
20137

(this is mentioned in the summary)

Sorry about that, I must've skipped that part.

This revision was automatically updated to reflect the committed changes.