This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Ensure wrapped ObjC selectors with 1 arg obey IndentWrappedFunctionNames
ClosedPublic

Authored by benhamilton on Mar 28 2018, 12:24 PM.

Details

Summary

In D43121, @Typz introduced logic to avoid indenting 2-or-more
argument ObjC selectors too far to the right if the first component
of the selector was longer than the others.

This had a small side effect of causing wrapped ObjC selectors with
exactly 1 argument to not obey IndentWrappedFunctionNames:

- (aaaaaaaaaa)
aaaaaaaaaa;

This diff fixes the issue by ensuring we align wrapped 1-argument
ObjC selectors correctly:

- (aaaaaaaaaa)
    aaaaaaaaaa;

Test Plan: New tests added. Test failed before change, passed

after change. Ran tests with:
% make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

Diff Detail

Repository
rL LLVM

Event Timeline

benhamilton created this revision.Mar 28 2018, 12:24 PM
jolesiak accepted this revision.Mar 29 2018, 12:02 AM

Well spotted.

This revision is now accepted and ready to land.Mar 29 2018, 12:02 AM
djasper added inline comments.Mar 29 2018, 5:27 AM
lib/Format/ContinuationIndenter.cpp
899 ↗(On Diff #140122)

I think I'd now find this slightly easier to read as:

unsigned MinIndent = State.Stack.back().Indent;
if (Style.IndentWrappedFunctionNames)
  MinIndent = std::max(MinIndent, State.FirstIndent + Style.ContinuationIndentWidth);
904 ↗(On Diff #140122)

Does this if actually change the behavior in any way? With LongestObjCSelectorName being 0, isn't that:

return MinIndent +
       std::max(0, ColumnWidth) - ColumnWidth;

(and with ColumnWidth being >= 0, this should be just MinIndent)

benhamilton marked 2 inline comments as done.

@djasper comments

Thanks, applied your suggested change.

lib/Format/ContinuationIndenter.cpp
899 ↗(On Diff #140122)

OK, done.

904 ↗(On Diff #140122)

The - ColumnWidth part is only for the case where LongestObjCSelectorName is *not* 0. If it's 0, we return MinIndent which ensures we obey Style.IndentWrappedFunctionNames.

The problem with the code before this diff is when LongestObjCSelectorName was 0, we ignored Style.IndentWrappedFunctionNames and always returned State.Stack.back().Indent regardless of that setting.

After this diff, when LongestObjCSelectorName is 0 (i.e., this is either the first part of the selector or a selector which is not colon-aligned due to block formatting), we change the behavior to indent to at least State.FirstIndent + Style.ContinuationIndentWidth, like all other indentation logic in this file.

I've added some comments explaining what's going on, since this code is quite complex.

One more tidy-up

djasper added inline comments.Mar 29 2018, 8:54 AM
lib/Format/ContinuationIndenter.cpp
904 ↗(On Diff #140122)

I am not saying your change is wrong. And I might be getting out of practice with coding. My question is, what changes if you remove lines 906 and 907 (the "if (...) return MinIndent")?

Remove if (...) return MinIndent.

benhamilton marked an inline comment as done.Mar 29 2018, 9:14 AM
benhamilton added inline comments.
lib/Format/ContinuationIndenter.cpp
904 ↗(On Diff #140122)

Oh, I see what you're saying now! Thanks for clarifying.

Yes, we can remove these lines now. Done.

djasper accepted this revision.Mar 30 2018, 8:03 AM

Looks good.

This revision was automatically updated to reflect the committed changes.
benhamilton marked an inline comment as done.
This revision was automatically updated to reflect the committed changes.