Page MenuHomePhabricator

[clang-format] restore indent in conditionals when AlignOperands is DontAlign
ClosedPublic

Authored by krasimir on Jun 19 2020, 8:19 AM.

Details

Summary

After D50078, we're experiencing unexpected un-indent using a style combining AlignOperands: DontAlign with BreakBeforeTernaryOperators: false, such as Google's JavaScript style:

% bin/clang-format -style=google ~/test.js
aaaaaaaaaaa = bbbbbbbb ? cccccccccccccccccc() :
dddddddddd             ? eeeeeeeeeeeeee :
                         fffff;

The issue lies with the interaction of AlignOperands: DontAlign and the edited code section in ContinuationIndenter.cpp, which de-dents the intent by Style.ContinuationIndentWidth. From the documentation of AlignOperands: DontAlign:

The wrapped lines are indented ContinuationIndentWidth spaces from the start of the line.

So the de-dent effectively erases the necessary ContinuationIndentWidth in that case.

This patch restores the AlignOperands: DontAlign behavior, producing:

% bin/clang-format -style=google ~/test.js
aaaaaaaaaaa = bbbbbbbb ? cccccccccccccccccc() :
    dddddddddd         ? eeeeeeeeeeeeee :
                         fffff;

Diff Detail

Event Timeline

krasimir created this revision.Jun 19 2020, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 8:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
krasimir edited the summary of this revision. (Show Details)Jun 19 2020, 8:33 AM
krasimir retitled this revision from [clang-format] restore indent in conditionals AlignOperands is DontAlign to [clang-format] restore indent in conditionals when AlignOperands is DontAlign.Jun 19 2020, 8:53 AM
sammccall accepted this revision.Jun 24 2020, 2:47 AM

This may not be the right long-term solution, but the current behavior is pretty broken and this is cheap.

clang/unittests/Format/FormatTest.cpp
6287

aligning the question marks here is a bit weird (given DontAlign) but that's another patch.

If we disable the question-column behavior with dontalign, this patch will be completely dead, right?
May want to add a FIXME to remove in that case.

This revision is now accepted and ready to land.Jun 24 2020, 2:47 AM
krasimir updated this revision to Diff 272957.Jun 24 2020, 3:36 AM
krasimir marked an inline comment as done.
  • add a FIXME
krasimir updated this revision to Diff 272964.Jun 24 2020, 3:52 AM
  • refresh Phabricator diff
krasimir updated this revision to Diff 272966.Jun 24 2020, 3:54 AM
  • try to update Phabricator diff
This revision was automatically updated to reflect the committed changes.
Typz added a subscriber: Typz.Jun 24 2020, 10:17 PM
Typz added inline comments.
clang/unittests/Format/FormatTest.cpp
6287

I don't think this is so weird: even with DontAlign, there are other cases when some alignment is performed: for exemple when formatting tables in column.

As I see it, ternary operator formatting is a similar case of 2D formatting, and while it needs indeed to respect the "general" line wrapping/indent mode (as per AlignOperands), it is OK to keep the alignment of the ternary operator themselves : otherwise, the whole "mode" for ternary operators need to be disabled and makes no sense.

But anyway this is a different patch as you mentioned, and maybe some user of DontAlign can come up with a better approach for formatting ternary ops in that case.

krasimir marked an inline comment as done.Jun 26 2020, 2:25 AM
krasimir added inline comments.
clang/unittests/Format/FormatTest.cpp
6287

Added a FIXME. I think disabling question-column alignment with dontalign indeed can make this patch obsolete.

6287

Agree, this is more of a policy / preference on how we want these snippets to look like with DontAlign. We should discuss it in more detail whenever we attempt to refine this later. In a way, this touches back on djasper's comment on D50078, which I believe was left unaddressed:

I don't think the alignment of "?" and ":" (in the WhitespaceManager) should be always on. I think we'd need a flag for that part