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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked an inline comment as done.
paulwalker-arm 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)
----------------
arichardson wrote:
> 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.
For what it's worth I took a run at an implementation that doesn't require a call prefix and whilst almost as simple as suggested above there are a couple of downsides.

(1) Calls without a prefix require look ahead parsing, which means redundant work continually looking for functions that might never be there. For example, take the parsing of var_a + var_b + var_c + var_d - (var_e - var_f) where parseNumericOperand is likely to perform many failed parse attempts.

(2) Some diagnostics become harder or impossible. For example, is "mu(la+b)" a call to an unknown function, a missing operator or a bracket typo. I know the same scenario is true if the user forgets the prefix but when they don't, we can emit a more useful message.  You can see another example in this patch where it's easier to spot and report a missing operator before a function call.

(3) A prefix allows the use of symbols that might otherwise be confusing.  Tenuous I know but consider "a + !operator+(b+c)".

(4) Are there any plans for VAR1(VAR2+VAR3) as short hand for mul(VAR1, VAR2+VAR3)?

A downside of the prefix is that we cannot easily use "!" to mean "not".  That said "!(var)" support is only a minor modification.

I don't know if these are strong reasons to go with a prefix and ultimately either approach works for me, so just let me know the preference and I'll make the necessary changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936