Page MenuHomePhabricator

[clang-format/ObjC] Fix counting selector name parts for ObjC
ClosedPublic

Authored by jolesiak on Jun 28 2018, 5:36 AM.

Details

Summary

Counts selector parts also for method declarations and counts correctly for methods without arguments.
This is an internal change and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719.

Diff Detail

Repository
rL LLVM

Event Timeline

jolesiak created this revision.Jun 28 2018, 5:36 AM
jolesiak updated this revision to Diff 153298.Jun 28 2018, 5:41 AM

Fix comment

jolesiak edited the summary of this revision. (Show Details)Jun 28 2018, 6:28 AM
jolesiak added reviewers: benhamilton, klimek.
benhamilton requested changes to this revision.Jun 28 2018, 8:26 AM

Count selector parts also for method declarations.

What bug does this fix? Can you add a test which breaks before this change and is fixed by this change?

lib/Format/FormatToken.h
247–248 ↗(On Diff #153298)

Grammar nit: that whole selector consist of -> that the whole selector consists of

lib/Format/TokenAnnotator.cpp
519 ↗(On Diff #153298)

Grammar nit-pick: when method -> when the method

520 ↗(On Diff #153298)

expression -> method call

627 ↗(On Diff #153298)

Grammar nit-pick: methods number -> methods, the number

628 ↗(On Diff #153298)

Grammar nit-picks:

  • have different structure -> have a different structure
  • parameters are not -> the parameters are not
628–629 ↗(On Diff #153298)

Why does parenthesis scope matter here? updateParameterCount() is called from parseSquare().

I'm not sure what the goal of this change is.

This revision now requires changes to proceed.Jun 28 2018, 8:26 AM
jolesiak marked 6 inline comments as done.Jul 2 2018, 2:27 AM
jolesiak added inline comments.
lib/Format/FormatToken.h
247–248 ↗(On Diff #153298)

Thanks!

lib/Format/TokenAnnotator.cpp
519 ↗(On Diff #153298)

Thanks!

520 ↗(On Diff #153298)

Thanks!

627 ↗(On Diff #153298)

Thanks!

628 ↗(On Diff #153298)

Thanks!

628–629 ↗(On Diff #153298)

My bad, I thought the word "parenthesis" can also mean any of "(", "[", "{", "<" (not only specifically the first one).
I've replaced with "a bracket scope".

I'll comment on the goal of this change in non-inline comment.

jolesiak marked 6 inline comments as done.Jul 2 2018, 2:54 AM

Count selector parts also for method declarations.

What bug does this fix? Can you add a test which breaks before this change and is fixed by this change?

Sorry for the confusion.

General comment to changes D48716, D48718, D48719, D48720 (which are the split of D48352):
These changes are separate, in the sense that they fix different issues. However, they should be chained as every change (apart from the first one) is based on previous ones: D48716 -> D48718 -> D48719 -> D48720. I don't know how to chain them in Phabricator (I should probably leave some comments about what every change is based on though).

D48716 fixes the mechanism of counting parameters. This is an internal change though and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719. Therefore, I'm not aware of any formatting test that fails before applying this change and succeeds after. As far as I know internal functions/methods are not tested at all. Thus, the tests are part of D48719.

This is why initially I put everything into one change: D48352, but I agree that it was not readable at all.

jolesiak updated this revision to Diff 153689.Jul 2 2018, 3:00 AM

Fix grammar mistakes.

benhamilton accepted this revision.Jul 2 2018, 9:58 AM

General comment to changes D48716, D48718, D48719, D48720 (which are the split of D48352):

These changes are separate, in the sense that they fix different issues. However, they should be chained as every change (apart from the first one) is based on previous ones: D48716 -> D48718 -> D48719 -> D48720. I don't know how to chain them in Phabricator (I should probably leave some comments about what every change is based on though).

Phabricator has an Edit Related Revisions feature where you can tag other revisions as being dependencies of or depending upon the current revision:

I went ahead and used it to link the revisions in the order you listed above. (I also like to edit the 1-line description and add [1/x] at the beginning — but that's up to you.)

D48716 fixes the mechanism of counting parameters. This is an internal change though and doesn't influence formatting on its own (at the current state). Its lack would be visible after applying D48719. Therefore, I'm not aware of any formatting test that fails before applying this change and succeeds after. As far as I know internal functions/methods are not tested at all. Thus, the tests are part of D48719.

Great. Just mentioning that in the diff description should be fine.

This revision is now accepted and ready to land.Jul 2 2018, 10:00 AM
jolesiak retitled this revision from [clang-format] Fix counting parameters/arguments for ObjC to [clang-format/ObjC] Fix counting selector name parts for ObjC.Jul 2 2018, 11:56 PM
jolesiak edited the summary of this revision. (Show Details)

Phabricator has an Edit Related Revisions feature where you can tag other revisions as being dependencies of or depending upon the current revision:

Thanks! That's what I was looking for.

This revision was automatically updated to reflect the committed changes.