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

[Thomas Preud'homme via Phabricator via llvm-commits]

thopre added inline comments.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:693
+  * a function call, or
+  * an expression followed by an operator and an expression.
 
----------------
paulwalker-arm wrote:
> thopre wrote:
> > paulwalker-arm wrote:
> > > thopre wrote:
> > > > Why change this sentence? Recursion only happens on one of the operand only.
> > > I don't understand the distinction. I changed the sentence as I didn't what to say:
> > > ```
> > > an expression followed by an operator and either a numerical operand or a function call.
> > > ```
> > > 
> > > Are you saying the above is more correct?
> > > 
> > > Also with functions you can have 
> > > ```
> > > !(mul(VAR+(umin(VAR2,4)) +  !(udivl(VAR3+(umax(VAR3,4))
> > > ```
> > Oh right, I forgot about function altogether. That said, I think an expression is a kind of numeric operand so we should just expand the definition below saying it can also be a function call.
> So something like:
> ```
> A numeric operand is a previously defined numeric variable, an integer literal or the result of a function call.
> ```
I'm nitpicking but technically the operand is the function call itself, but evaluates to the return value of the function call.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:708-709
+
+  ``!(<expr>)`` is a precedence operator, which can be used to force the order
+  of operations when symbolic operators are in use.
+
----------------
paulwalker-arm wrote:
> arichardson wrote:
> > thopre wrote:
> > > paulwalker-arm wrote:
> > > > thopre wrote:
> > > > > I don't think the exclamation mark should be required here. A parenthesis pair should be enough to force precedence. Note that there was a patch to adds support for that and you might want to rebase your patch on top of it.
> > > > Sure, it's more that this implementation comes for free, whereas supporting arbitrary parenthesis pairs along side function calls requires more complex parsing.  Is it worth the extra effort?  If so I'll happy prevent this usage and error out for unnamed functions.
> > > > 
> > > > I'd rather not wait for the parenthesis work because I've got other work at review whose tests are much improved when I can make use of function calls.
> > > I think the exclamation mark should be reserved for function call. I find it a bit confusing otherwise but let's wait to see what other reviewers might think.
> > > 
> > > Do the tests you need this feature for require some way to force precedence or can this be dealt later?
> > Apologies for the delay, I've now rebased the parentheses revision (D77383).
> I currently have no use for the precedence operator, the function calls are why I started down this path, so am happy either way.
> 
> Part of me just assumed that in the future the parser might be reworked to remove the need for the ! prefix, but I suppose providing the power of precedence early might prevent that work from happening :)
I wouldn't hold my breath for a parser rewrite. While I'd love to make the code nicer and more flexible I have little free time to work on it myself. Anyway, if you don't require operator precedence just error on empty function name for now and we can extend it to be used for operator precedence later if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936