aaron.ballman added inline comments.
================
Comment at: clang/lib/Parse/ParseObjc.cpp:749
+ if (!Tok.is(tok::eof))
+ ConsumeToken();
break;
----------------
rjmccall wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > danix800 wrote:
> > > > tbaeder wrote:
> > > > > Why is there a `ConsumeToken()` call at all here? The token is
> > > > > already being consumed in line 729.
> > > > Didn't notice this, thanks for reminding!
> > > I have the same question as @tbaeder -- what token is this intending to
> > > consume? CC @rjmccall for Obj-C expertise
> > OH! This is consuming the identifier for the implementation/interface name
> > itself. e.g.,
> > ```
> > @interface Frobble
> > ```
> > The consume on line 709 gets the `@`, the consume on line 729 gets the
> > `interface`, and the consume on line 749 is getting the `Frobble`. That
> > makes sense to me now.
> >
> I don't think any language expertise is required here — just seems like a
> straightforward bug on an error path that's probably not exercised all that
> often. Maybe somebody moved the `ConsumeToken` and forgot to fix this case
> or something.
What concerns me about this fix is that we don't typically check whether the
token is EOF or not before consuming; that's usually an anti-pattern, isn't it?
Wouldn't it make sense for this to use `SkipUntil(tok::identifier)` instead?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156277/new/
https://reviews.llvm.org/D156277
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
