[PATCH] D79936: [FileCheck] Add function call support to numerical expressions.

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:287
+  // Try to parse a function call.
+  if (Expr.startswith(CallPrefix)) {
+    if (AO != AllowedOperand::Any)
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > I'm not strongly opposed to the use of `!` to indicate a function call, but is it actually necessary? It seems like a function call could just be identified by `<sequence of identifier chars>(<some chars>)`. The code would look something like the following semi-pseudo code:
> > > 
> > > ```
> > > size_t Parenthesis = Expr.find_first_of('(');
> > > if (Parenthesis != npos) {
> > >   if (all_of(Expr.take_front(Parenthesis), [](char C) { 
> > >       return isValidIdentifierChar(C);
> > >     }) {
> > >     if (AO != AllowedOperand::Any)
> > >       return ErrorDiagnostic::get(SM, Expr, "unexpected function call");
> > > 
> > >     return parseCallExpr(Expr, LineNumber, Context, SM);
> > >   }
> > > }
> > > ```
> > > 
> > > Assuming I've not missed something, that would allow us to simplify all the usages of function calls.
> > Regardless of the ease of implementation, I like the ! prefix since these are builtin functions/operators, not something the user can define. YMMV of course
> I'm not sure why it matters that they are builtin? Even if we do provide the ability for users to define their own functions, surely their behaviour should be identical to built-in functions from the majority of the code's point-of-view? I'd actually think that including the `!` would make it harder to parse, since we'd have to support function calls both with and without the `!`.
If we can make it work without the `!` without making the implementation much more complicated, I'd prefer that.
But I don't feel strongly either way.

Since the name needs to be followed by an open paren, even variables that have the same name as builtin functions should work: `[[#mul(mul, 2)]]`. The first one is a function name, the second must be a variable since there is no open paren.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936