[PATCH] D15173: [Preprocessor] Fix assertion in AnnotatePreviousCachedTokens

[Richard Smith via cfe-commits]

rsmith added a subscriber: rsmith.
rsmith requested changes to this revision.
rsmith added a reviewer: rsmith.
rsmith added a comment.
This revision now requires changes to proceed.

I think that this will leave us with a broken token stream. In your example, the cached token stream starts as

`NSArray` `<` `id` `<` `PB` `>>` `*` [...]

... and we try to annotate the `id<PB>` with our `CachedLexPos` pointing at the `*` token. That leaves `CachedTokens` containing:

`NSArray` `<` `(type annotation)` `*` [...]

... which is wrong. We need to actually convert the `tok::greatergreater` in `CachedTokens` into a `tok::greater` and update its location and length in order for the cached token stream to be correctly updated. Otherwise if the parser backtracks it will see the wrong token stream.


================
Comment at: lib/Lex/PPCaching.cpp:102-104
@@ +101,5 @@
+#ifndef NDEBUG
+  Token CachedLastTok = CachedTokens[CachedLexPos - 1];
+  SourceLocation CachedLastTokLoc = CachedLastTok.getLastLoc();
+  SourceLocation TokAnnEndLoc = Tok.getAnnotationEndLoc();
+
----------------
Please add braces inside this #ifndef block so these variables don't accidentally leak into the rest of the function.

================
Comment at: lib/Lex/PPCaching.cpp:108
@@ +107,3 @@
+  // `Tok` length could be bigger than one (e.g. greatergreater '>>'), account
+  // for that cases before checking the assertion.
+  if (!CachedLastTok.isAnnotation()) {
----------------
that cases -> that case

================
Comment at: lib/Lex/PPCaching.cpp:109
@@ +108,3 @@
+  // for that cases before checking the assertion.
+  if (!CachedLastTok.isAnnotation()) {
+    CachedLastTokLoc =
----------------
Please only do this extra work if `CachedLastTokLoc != TokAnnEndLoc`.


http://reviews.llvm.org/D15173