[PATCH] D98691: [FileCheck] Fix PR49531: invalid use of string var

Thu Mar 18 02:31:01 PDT 2021

jhenderson accepted this revision.
jhenderson added a comment.

LGTM! I don't think there's a pressing need to get rid of the legacy pseudo variables, but I'm also not opposed to it, if you think it's worth doing.

================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:460
   StringRef Name = Str.take_front(I);
   Str = Str.substr(I);
   return VariableProperties {Name, IsPseudo};
----------------
thopre wrote:
> jhenderson wrote:
> > thopre wrote:
> > > jhenderson wrote:
> > > > I wonder if it would make more sense to add the check to this function, somewhere around here. This is the "parseVariable" function after all, so seems like the logical place to reject invalid variable names, rather than the place it is currently detected. What do you think?
> > > A trailer has to be allowed when parsing a variable use because it could be `[[@LINE+1]]`. If the variable is a pseudo variable, parsing is deferred to the numeric substitution block parsing. Otherwise any trailer is an error. Perhaps it's time to remove all uses of `[[@LINE]]` in favor of `[[#@LINE]]`
> > This function already knows whether it is a pseudo variable, so doesn't that mean you can do something like `if(!IsPseudo) /*error if trailer*/`?
> parseVariable is also called for numeric expression where it's fine to have a trailer after a variable (`[[#FOO+2]]`). I would need to distinguish 3 states:
> 
> - No trailer (e.g. string variable def and use)
> - Trailer for pseudo only (Legacy @LINE expression)
> - Trailer allowed (numeric expression)
> 
> That's why I suggested removing legacy @LINE expression, it would reduce this to 2 options which can be dealt with a AllowTrailer (false by default) parameter.
Ah, thanks. Didn't realise this was used elsewhere.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98691/new/

https://reviews.llvm.org/D98691