This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix formatting of if statements with BlockIndent
Needs ReviewPublic

Authored by gedare on Jul 7 2023, 4:28 PM.

Details

Summary

A bug with BlockIndent prevents line breaks within if (and else if) clauses.
While fixing this bug, it appears that AlignAfterOpenBracket is not designed
to work with loop and if statements, but AlwaysBreak works on if clauses.
The documentation and tests are not clear on whether or not this is intended.
This patch preserves the AlwaysBreak behavior but does not use BlockIndent on if
clauses.

It may be reasonable to create a new style option for alignment of if (and loop) clauses intentionally so AlwaysBreak and BlockIndent can be enabled or disabled for conditional blocks.

Fixes #54663.

Diff Detail

Event Timeline

gedare created this revision.Jul 7 2023, 4:28 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJul 7 2023, 4:28 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
gedare requested review of this revision.Jul 7 2023, 4:28 PM
gedare added a comment.Jul 7 2023, 4:35 PM

Although I chose to preserve the existing behavior of AlwaysBreak and to mimic it for BlockIndent, I think it would be best to choose a consistent approach and document it. I also have a patch that does not apply the AlignAfterOpenBracket for if clauses, which could be done for only BlockIndent, or it could be done for AlwaysBreak too but that will cause some backward compatibility problems.

clang/unittests/Format/FormatTest.cpp
25486

Why remove?

gedare updated this revision to Diff 541813.Jul 18 2023, 7:14 PM
gedare marked an inline comment as done.

Revert deleted line in test.

clang/unittests/Format/FormatTest.cpp
25486

oops. that's a spurious change. I have reverted it.

MyDeveloperDay added a comment.EditedJul 19 2023, 1:44 AM

It looks like this was added in D109557: Adds a BlockIndent option to AlignAfterOpenBracket I'm trying to understand what bad stuff happens if you remove the "if" from

return !(Previous && Previous->is(tok::kw_for));

@csmulhern any thoughts? (if you are still around?)

MyDeveloperDay requested changes to this revision.Jul 19 2023, 1:52 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/FormatTest.cpp
25504

we don't do this.

25505–25510

you can't remove a test and just call it good... the original author put this test in for a reason I assume?

25514

isn't the breaking of if ( and } else if (\n inconsistent?

This revision now requires changes to proceed.Jul 19 2023, 1:52 AM
gedare added inline comments.Jul 19 2023, 8:13 AM
clang/unittests/Format/FormatTest.cpp
25504

OK, I'll move that other bug up my priority list, as the behavior of this test case involves both. That bug prevents breaking the line after the operand, I think there's some off-by-1 error somewhere in calculating the columns for blockindent breaks. Otherwise, this patch reveals that bug as a regression in this test case.

25505–25510

understood, although it seems that the original author may not have considered if statements as legitimately to be used in BlockIndented formats, since this test case does not block indent. So, the test case will have to change anyway, if the if should be blockindented.

25514

The breaking is dependent on what ends up exceeding the ColumnLimit. since if ( is shorter than } else if ( by about 5 characters, you can get different breaks. As far as I can tell, the behavior of AlwaysBreak and BlockIndent will only break after the opening parens if the first "argument" doesn't fit. From what I understand, the penalties and desirable breaks come into play then, based on breaking around commas or boolean operators. With this patch, the behavior here is now consistent for both AlwaysBreak and BlockIndent.

This behavior is undocumented and I do not know the intention. I would think it is better for the two options to behave consistently, but I'm also willing to treat them differently for backward compatibility while fixing this bug.

I guess since the current behavior does not treat if with block indentation, this bug fix should instead prefer to maintain the current behavior, and if someone wants to have block indentation of if it would need to be added as a new style option. Otherwise, this will introduce regressions like that test case.

gedare updated this revision to Diff 542625.Jul 20 2023, 11:54 AM

Do not apply BlockIndent to if conditionals. Address other comments.

gedare marked 3 inline comments as done.Jul 20 2023, 11:57 AM

I have fixed the bug in a different way that avoids using BlockIndent on if conditionals. AlwaysBreak will continue to apply to if conditionals by default. I may propose a style option to control that behavior later.

gedare edited the summary of this revision. (Show Details)Jul 20 2023, 11:58 AM
gedare updated this revision to Diff 558063.Nov 9 2023, 7:50 AM

Rebase to main

gedare added a comment.Nov 9 2023, 7:53 AM

@MyDeveloperDay ping

I have rebased to main without any problems. This patch fixes the bug.

owenpan added inline comments.Nov 9 2023, 2:45 PM
clang/lib/Format/ContinuationIndenter.cpp
773–792 ↗(On Diff #558063)

This is getting out of hand. Consider writing a lambda with early returns.

owenpan added inline comments.Nov 9 2023, 2:50 PM
clang/unittests/Format/FormatTest.cpp
25513–25523

Please use camel case.

gedare updated this revision to Diff 558069.Nov 9 2023, 3:08 PM

Use cameLCAse in test

gedare marked an inline comment as done.Nov 9 2023, 3:09 PM
gedare added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
773–792 ↗(On Diff #558063)

I would probably not get to refactoring this until January.