Page MenuHomePhabricator

clang-format incorrectly indents wrapped closing parenthesis
ClosedPublic

Authored by owenpan on Apr 6 2019, 11:00 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

owenpan created this revision.Apr 6 2019, 11:00 PM
MyDeveloperDay added inline comments.Apr 7 2019, 5:40 AM
clang/lib/Format/ContinuationIndenter.cpp
968 ↗(On Diff #194052)

Do you think the colon you have here in your example is really a TT_CtorInitializerColon, I'm not sure if you need to be specific?

I wonder how this might interfere with the Style.BreakConstructorInitializers, I'm a little unclear of the before and after you are trying to fix, could you add something to the description to help me understand?

klimek added a comment.Apr 7 2019, 7:57 AM

The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable.

maybe add the following as a test because I think it shows the inconsistency

void Foo::bar( // some comment
) {}

void Foo::bar( // some comment
) const {}

The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable.

I see the bug in the inconsistency for the code below:

int Foo::getter(
    //
) const {
  return foo;
}

void Foo::setter(
    //
) {
  foo = 1;
}

The closing parenthesis is indented in getter but not setter.

maybe add the following as a test because I think it shows the inconsistency

void Foo::bar( // some comment
) {}

void Foo::bar( // some comment
) const {}

I had the setter code in the new test case to show the inconsistency but then took it out; I don't want to add a (possible) duplicate even though I didn't find the test case for Lines 953-957 of ContinuationIndenter.cpp. I will put my setter test case back in.

MyDeveloperDay accepted this revision.Apr 7 2019, 12:48 PM

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

This revision is now accepted and ready to land.Apr 7 2019, 12:48 PM
owenpan planned changes to this revision.Apr 7 2019, 12:51 PM
owenpan added inline comments.
clang/lib/Format/ContinuationIndenter.cpp
968 ↗(On Diff #194052)

Good point with using TT_CtorInitializerColon instead!

Although I had tested it with different combinations of Style.BreakConstructorInitializers settings and didn't find any interference, it's probably not an inconsistency in the case of the colon based on your and @klimek's comments.

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

Thanks! I just got the alternative patch ready. Should I discard it and just commit this one?

LGTM , if you also think the test will help show the use case then please add it, otherwise this revision notes might be information enough

Thanks! I just got the alternative patch ready. Should I discard it and just commit this one?

Maybe just update this revision with the new diff.

owenpan updated this revision to Diff 194072.Apr 7 2019, 1:40 PM
This revision is now accepted and ready to land.Apr 7 2019, 1:40 PM
owenpan requested review of this revision.Apr 7 2019, 1:42 PM

Updated the diff.

MyDeveloperDay accepted this revision.Apr 7 2019, 1:50 PM
This revision is now accepted and ready to land.Apr 7 2019, 1:50 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2019, 2:05 PM