This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Fix indentation for array variables with alignment of consecutive assignments and declarations.
ClosedPublic

Authored by curdeius on Jan 3 2022, 1:41 AM.

Diff Detail

Event Timeline

curdeius requested review of this revision.Jan 3 2022, 1:41 AM
curdeius created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 1:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I know I changed existing tests. I'm not sure what we really want here.
If you have ideas, I'm open to suggestions.

The fix looks ok, butr I don't understand the cause of the side effect? is it explicitly aligning to the v of auto v?

The fix looks ok, butr I don't understand the cause of the side effect? is it explicitly aligning to the v of auto v?

It's adding two spaces inside the auto v = ..., because of aligned declaration (identifier v, additional 1 space) and aligned assignment operator (=, 1 more space).

Unless the test cases are wrong, they should not be changed. The original test case looked correct to me as it was doing a continuation indent between type{ and }.

This is born out by setting ContinuationIndentWidth: 8

int   i  = 0;
float i2 = 0;
auto  v  = type{
        i = 1,   //
        (i = 2), //
        i = 3    //
};

So maybe the original test is correct..

Could we resolve this with just

// Continued braced list.
if (Changes[ScopeStart - 2].Tok->isNot(tok::identifier) &&
    Changes[ScopeStart - 1].Tok->is(tok::l_brace) &&
    Changes[i].Tok->isNot(tok::r_brace))
  return true;
curdeius updated this revision to Diff 397265.Jan 4 2022, 4:30 AM

Reverted test changes. Checking for identifier before brace.

owenpan accepted this revision.Jan 4 2022, 10:44 AM
This revision is now accepted and ready to land.Jan 4 2022, 10:44 AM
This revision was landed with ongoing or failed builds.Jan 5 2022, 4:52 AM
This revision was automatically updated to reflect the committed changes.
hans added a subscriber: hans.Apr 28 2022, 2:25 AM

This seems to have caused a strange indentation problem, see https://github.com/llvm/llvm-project/issues/55161

Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2022, 2:25 AM

Feel free to revert and add a regression test please. I'll take a look.

hans added a comment.Apr 28 2022, 2:42 AM

Feel free to revert and add a regression test please. I'll take a look.

Since this patch landed a couple of months back, I don't think we need to rush to revert it.