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

Thu Jun 4 01:02:30 PDT 2020

jhenderson added a comment.

Taking conversation out-of-line to make it easier. My personal beliefs are as follows:

> (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.

By look ahead parsing are you saying something like to identify whether `var_a` is actually a function, we have to parse the `+`? I've not given too much thought to this, but I think this extra work can be avoided by delaying handling of a token until the next token is identified. Thus an identifier token is left unprocessed until the next token has been read in, at which point it is either processed as a function or a variable. However, I accept that might need some rewriting of the existing parsing code. Trying to parse something as a numeric operand as a first attempt before trying to read it as something else seems like the wrong approach long-term as we add more power to these expressions.

> (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.

I think it's okay in that case to treat that as an attempt to call function `mu`, which probably doesn't exist. I'm not sure having the prefix helps in this case: `!mu(la+b)` is just as unknown a function. The function name is delimited by the end of the previous token and the opening parenthesis in the unprefixed case. I'm not sure I see the case where it's easier to spot a missing operand. The examples in the patch are (I think):

- `(1)(2)` - this is unaffected - no identifier means these are treated as parenthesised expressions.
- `2(X)` - 2 will be parsed as a numeric literal, so this is still a missing operator.
- `2!mul(FOO,2)` - without the `!`, either the 2 is treated as a separate token, because it can't be the first character of an identifier (and therefore again a missing operator diagnostic is still possible), or `2mul` becomes an invalid identifier, with corresponding message. I think either is an acceptable error.
- `FOO !mul(FOO,2)` - without the `!`, FOO is still a separate token because of the whitespace, so the missing operand is easily identifiable.

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

I think we either a) don't need to support such expressions or b) special-case + following the term `operator` or equivalent sequences. I think we just want to keep our function names to valid identifiers like we already have for variable names.

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

I'm not aware of any plans, and I don't think they're really needed (especially if we add support for `*` as a binary operator for multiplication).

> 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 think we have any plans or need for boolean support (tools don't generally print "true" or "false" and other things like 1 or 0 can be supported using regular numeric expressions). We will probably want to support `!=` as a comparator though at some point, so we need to allow for that.

> 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.

My personal preference is still no prefix. Related aside is this comic: https://xkcd.com/1306/ (especially the alt text).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936