shafik added inline comments.
================
Comment at: clang/lib/Parse/ParseTentative.cpp:1553-1554
return TPResult::Error;
- if (Tok.isNot(tok::identifier))
+ if (NextToken().isNot(tok::identifier))
break;
}
----------------
rsmith wrote:
> shafik wrote:
> > cor3ntin wrote:
> > > rsmith wrote:
> > > > This doesn't seem correct to me. If we had `scope::foo bar`, and we
> > > > annotate `scope::foo` as a type, then this will get confused by the
> > > > next token now being an (unrelated) identifier. This code is trying to
> > > > detect if an annotation was performed, so I think it intended to check
> > > > if the current token's kind has changed, like is done on line 1295.
> > > The confusing bit is that Tok is always an annotated scope already here
> > > (L1598), so TryAnnotateName should not modify that first token (unless
> > > TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current
> > > annot_cxxscope by another one, which i don't think can happen?)
> > Ok using `tok::annot_cxxscope` also works and I agree it makes sense as
> > well, `check-clang` also passes.
> >
> > So then is the assert below wrong?
> >
> > ```
> > // Annotated it, check again.
> > assert(Tok.isNot(tok::annot_cxxscope) ||
> > NextToken().isNot(tok::identifier));
> > ```
> >
> > It looks like it will work by accident for most cases b/c it checks
> > `tok::annot_cxxscope` first.
> > The confusing bit is that Tok is always an annotated scope already here
> > (L1598), so TryAnnotateName should not modify that first token (unless
> > TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current
> > annot_cxxscope by another one, which i don't think can happen?)
>
> Yeah, I think `TryAnnotateTypeOrScopeToken` shouldn't ever replace an
> `annot_cxxscope` token with a different `annot_cxxscope` token representing a
> longer scope specifier -- an `annot_cxxscope` token should always be as long
> as it can be. But it might replace the `annot_cxxscope` token with an
> `annot_typename`, in which case we want to jump out to line 1671 and try
> again.
>
> > So then is the assert below wrong?
>
> I think it's right -- we either reach the assert if we replace the
> `annot_cxxscope` with something else (an `annot_typename`), in the
> `ANK_TemplateName` case, or if we've successfully annotated the name (as one
> of various non-identifier things), in the `ANK_Success` case. In either case,
> we only reach the assert if we successfully replaced the identifier with an
> annotation token, so the assert should succeed.
>
> And the point of the assert, I think, is to ensure that the recursive call to
> `isCXXDeclarationSpecifier` cannot reach this same codepath again and recurse
> forever, so checking the same condition that we checked on entry seems
> appropriate.
> > The confusing bit is that Tok is always an annotated scope already here
> > (L1598), so TryAnnotateName should not modify that first token (unless
> > TryAnnotateTypeOrScopeTokenAfterScopeSpec can somehow replace the current
> > annot_cxxscope by another one, which i don't think can happen?)
>
> Yeah, I think `TryAnnotateTypeOrScopeToken` shouldn't ever replace an
> `annot_cxxscope` token with a different `annot_cxxscope` token representing a
> longer scope specifier -- an `annot_cxxscope` token should always be as long
> as it can be. But it might replace the `annot_cxxscope` token with an
> `annot_typename`, in which case we want to jump out to line 1671 and try
> again.
I see the code that can generate `annot_typename` but I am so far not able to
come up w/ a scenario that hits that case. So I am a little hesitant to handle
that w/o adding a test that covers it. Although using
```
if (Tok.isNot(tok::annot_cxxscope) || Tok.is(tok::annot_typename))
break;
```
does pass `check-clang`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D134334/new/
https://reviews.llvm.org/D134334
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
