shafik marked an inline comment as done.
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;
             }
----------------
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. 


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

Reply via email to