This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] New format param ObjCBinPackProtocolList
ClosedPublic

Authored by benhamilton on Jan 29 2018, 9:58 AM.

Details

Summary

This is an alternative approach to D42014 after some
investigation by stephanemoore@ and myself.

Previously, the format parameter BinPackParameters controlled both
C function parameter list bin-packing and Objective-C protocol conformance
list bin-packing.

We found in the Google style, some teams were changing
BinPackParameters from its default (true) to false so they could
lay out Objective-C protocol conformance list items one-per-line
instead of bin-packing them into as few lines as possible.

To allow teams to use one-per-line Objective-C protocol lists without
changing bin-packing for other areas like C function parameter lists,
this diff introduces a new LibFormat parameter
ObjCBinPackProtocolList to control the behavior just for ObjC
protocol conformance lists.

The new parameter is an enum which defaults to Auto to keep the
previous behavior (delegating to BinPackParameters).

Depends On D42649

Test Plan: New tests added. make -j12 FormatTests && ./tools/clang/unittests/Format/FormatTests

Diff Detail

Repository
rC Clang

Event Timeline

benhamilton created this revision.Jan 29 2018, 9:58 AM
krasimir edited reviewers, added: djasper; removed: krasimir.Jan 29 2018, 11:40 AM
stephanemoore requested changes to this revision.Jan 30 2018, 1:43 PM
stephanemoore added inline comments.
lib/Format/ContinuationIndenter.cpp
1232

Given that we now have a new setting distinguishing bin packing of Objective-C protocol conformance lists and bin packing of parameters perhaps this local variable should be named more generally? Maybe "BinPackDeclaration" would be appropriate because this local variable takes effect if State.Line->MustBeDeclaration is true?

lib/Format/Format.cpp
766

I propose that we defer this to a future commit while we establish consensus at Google.

unittests/Format/FormatTestObjC.cpp
331–332

Similar to my other comment, I propose deferring the changes to Google style to a future commit.

This revision now requires changes to proceed.Jan 30 2018, 1:43 PM

Move Google style changes out. Use clearer name for local variable.

benhamilton edited the summary of this revision. (Show Details)Jan 30 2018, 1:56 PM
benhamilton marked 3 inline comments as done.Jan 30 2018, 1:57 PM

Thanks for the review!

lib/Format/Format.cpp
766

Moved to D42708.

unittests/Format/FormatTestObjC.cpp
331–332

Moved to D42708.

benhamilton marked 2 inline comments as done.
  • BinPackObjCProtocolList -> ObjCBinPackProtocolList
benhamilton retitled this revision from [clang-format] New format param BinPackObjCProtocolList to [clang-format] New format param ObjCBinPackProtocolList.Jan 31 2018, 8:44 AM
benhamilton edited the summary of this revision. (Show Details)
stephanemoore accepted this revision.Jan 31 2018, 3:10 PM

Looks good to me 👌 Thanks for driving this!

This revision is now accepted and ready to land.Jan 31 2018, 3:10 PM

Thanks! @jolesiak is not available for the rest of the week, so should I just submit this as-is, or wait until next week?

stephanemoore added inline comments.Feb 1 2018, 8:36 PM
lib/Format/ContinuationIndenter.cpp
1214

With the new parameter does this comment still apply? Maybe we can remove it?

benhamilton added inline comments.Feb 2 2018, 8:25 AM
lib/Format/ContinuationIndenter.cpp
1214

Good point.

This is about <> brackets nested inside () parens, which I believe is applicable to Objective-C 2.0 generics passed to C/C++ functions, but not to protocol conformance lists.

I'll send out a separate diff to add tests and clean up this comment, since I think the behavior is wanted for ObjC as well, but I think it's unrelated to this diff.

This revision was automatically updated to reflect the committed changes.
benhamilton marked 2 inline comments as done.Feb 2 2018, 12:33 PM
benhamilton added inline comments.
lib/Format/ContinuationIndenter.cpp
1214

Followup in D42864.

I don't understand why do we introduce an enum option if we are keeping the default behavior for Google style. IMO we should have a single behavior for any style and enforce it.

benhamilton marked an inline comment as done.Feb 6 2018, 9:37 AM

I don't understand why do we introduce an enum option if we are keeping the default behavior for Google style. IMO we should have a single behavior for any style and enforce it.

D42708 changes the default behavior for the Google style.

I'm fine with opening up a discussion to make this the default for all styles, but I think that should be a separate discussion.