rsmith 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; } ---------------- 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. 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