Page MenuHomePhabricator

[clang-format] Refine ObjC guesser to handle child lines of child lines

Authored by benhamilton on Mar 23 2018, 8:03 AM.



This fixes an issue brought up by djasper@ in his review of D44790. We
handled top-level child lines, but if those child lines themselves
had child lines, we didn't handle them.

Rather than use recursion (which could blow out the stack), I use a
DenseSet to hold the set of lines we haven't yet checked (since order
doesn't matter), and update the set to add the children of each
line as we check it.

Test Plan: New tests added. Confirmed tests failed before fix

and passed after fix.

Diff Detail

rC Clang

Event Timeline

benhamilton created this revision.Mar 23 2018, 8:03 AM

Update from correct base revision

djasper added inline comments.Mar 26 2018, 1:33 AM

Wouldn't it be much easier to call this function recursively for Children instead of using the lambda as well as this additional set? Lines and their children should form a tree, I think, so you should never see the same line again during recursion.

benhamilton added inline comments.Mar 26 2018, 8:23 AM

I'm less worried about cycles and more worried about a maliciously deeply nested set of children blowing out the stack and causing a crash.

It seemed safer to use the heap here to avoid that scenario (I don't know of any other way to avoid it, since we don't know the stack size and we don't control it.)

benhamilton added inline comments.Mar 26 2018, 8:56 AM

djasper@ informed me that clang-format is already full of places which rely on the stack for recursion on user input, so that window has already been broken. :)

I'll happily go ahead and change this.

  • Use recursion. Split lambda out into its own static method since recursion on lambdas is quite ugly until C++14
  • Remove stray semicolon.
djasper added inline comments.Mar 27 2018, 2:07 AM

I would not create a second function here. Just iterate over the tokens here and call guessIsObjC recursively with Line->Children. That means we need one less for loop overall, I think (I might be missing something).


Convention would be lineContainsObjCCode.

benhamilton marked 2 inline comments as done.
  • Simplify recursion. Remove static method LineContainsObjCCode().

Good call, done.

I don't know why I didn't do that first! For some reason, my brain thought Line.Children was an incompatible type..



djasper accepted this revision.Mar 27 2018, 7:54 AM

Looks good.

This revision is now accepted and ready to land.Mar 27 2018, 7:54 AM
This revision was automatically updated to reflect the committed changes.