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

[Richard Smith via cfe-commits]

On Jan 7, 2016 7:13 PM, "Bruno Cardoso Lopes" <bruno.cardoso at gmail.com>
wrote:
>
> bruno updated this revision to Diff 44306.
> bruno added a comment.
>
> Hi Richard,
>
> Thanks for the detailed explanation, it now makes sense to me. I updated
the patch with another approach! Let me know if it's the right direction.
>
>
> http://reviews.llvm.org/D15173
>
> Files:
>   include/clang/Lex/Preprocessor.h
>   lib/Lex/PPCaching.cpp
>   lib/Parse/ParseTemplate.cpp
>   test/Parser/objcxx11-protocol-in-template.mm
>
> Index: test/Parser/objcxx11-protocol-in-template.mm
> ===================================================================
> --- test/Parser/objcxx11-protocol-in-template.mm
> +++ test/Parser/objcxx11-protocol-in-template.mm
> @@ -8,3 +8,11 @@
>
>  vector<id<P>> v;
>  vector<vector<id<P>>> v2;
> +
> + at protocol PA;
> + at protocol PB;
> +
> + at class NSArray<ObjectType>;
> +typedef int some_t;
> +
> +id<PA> FA(NSArray<id<PB>> *h, some_t group);
> Index: lib/Parse/ParseTemplate.cpp
> ===================================================================
> --- lib/Parse/ParseTemplate.cpp
> +++ lib/Parse/ParseTemplate.cpp
> @@ -827,6 +827,7 @@
>    }
>
>    // Strip the initial '>' from the token.
> +  Token PrevTok = Tok;
>    if (RemainingToken == tok::equal && Next.is(tok::equal) &&
>        areTokensAdjacent(Tok, Next)) {
>      // Join two adjacent '=' tokens into one, for cases like:
> @@ -843,6 +844,20 @@
>                                                   PP.getSourceManager(),
>                                                   getLangOpts()));
>
> +  // The advance from '>>' to '>' in a ObjectiveC template argument list
needs
> +  // to be properly reflected in the token cache to allow correct
interaction
> +  // between annotation and backtracking.
> +  if (ObjCGenericList && PrevTok.getKind() == tok::greatergreater &&

Why do you only do this in one special case, rather than in the general
case where we split a token?

> +      RemainingToken == tok::greater &&
> +      PrevTok.getLocation().getRawEncoding() <=
> +          PP.getLastCachedTokenLocation().getRawEncoding()) {

A <= check on a source location raw encoding is pretty much meaningless if
you don't know they have the same FileID (and even then, you should ask
SourceManager to compare the locations).

Can you push the "is this a cached token" check down into the preprocessor
instead? (If we're doing token caching, how can it not be?)

> +    Token ReplTok = PrevTok;
> +    PrevTok.setKind(RemainingToken);
> +    PrevTok.setLength(1);
> +    SmallVector<Token, 2> NewToks = {PrevTok, Tok};
> +    PP.ReplaceCachedToken(ReplTok, NewToks);
> +  }
> +
>    if (!ConsumeLastToken) {
>      // Since we're not supposed to consume the '>' token, we need to push
>      // this token and revert the current token back to the '>'.
> Index: lib/Lex/PPCaching.cpp
> ===================================================================
> --- lib/Lex/PPCaching.cpp
> +++ lib/Lex/PPCaching.cpp
> @@ -116,3 +116,19 @@
>      }
>    }
>  }
> +
> +void Preprocessor::ReplaceCachedToken(const Token &Tok,
> +                                      SmallVectorImpl<Token> &NewToks) {

It seems like this could always assume that it's replacing the most recent
token (the one at CachedLexPos-1).

> +  assert(CachedLexPos != 0 && "Expected to have some cached tokens");
> +  for (auto i = CachedLexPos - 1; i != 0; --i) {
> +    const Token CurrCachedTok = CachedTokens[i];
> +    if (CurrCachedTok.getKind() == Tok.getKind() &&
> +        CurrCachedTok.getLocation() == Tok.getLocation()) {
> +      CachedTokens.insert(CachedTokens.begin() + i, NewToks.begin(),
> +                          NewToks.end());
> +      CachedTokens.erase(CachedTokens.begin() + i + NewToks.size());
> +      CachedLexPos += NewToks.size() - 1;
> +      return;
> +    }
> +  }
> +}
> Index: include/clang/Lex/Preprocessor.h
> ===================================================================
> --- include/clang/Lex/Preprocessor.h
> +++ include/clang/Lex/Preprocessor.h
> @@ -1185,6 +1185,12 @@
>      return CachedTokens[CachedLexPos-1].getLastLoc();
>    }
>
> +  /// \brief Replace token \p Tok in CachedTokens by the tokens in \p
NewToks.
> +  ///
> +  /// Useful when a token needs to be split in smaller ones and
CachedTokens
> +  /// must to be updated to reflect that.
> +  void ReplaceCachedToken(const Token &Tok, SmallVectorImpl<Token>
&NewToks);
> +
>    /// \brief Replace the last token with an annotation token.
>    ///
>    /// Like AnnotateCachedTokens(), this routine replaces an
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160118/ac0ea179/attachment.html>