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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

A couple of test cases that might want adding:

1. Trying to use a variable as a function (e.g. `VAR1(1, 2)`)
2. Trying to use a function as a variable (e.g. `max + min`)
3. Maybe even defining a variable explicitly as a recognised function name (e.g. `max + max(1, 2)` or even `max + max(max, max)`).

I reckon the first should be treated as an unrecognised variable, and the others allowed (although the second one probably would be using undefined variables).



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:707
+
+* ``name`` is a predefined string literal. Acceptable values are:
+
----------------
Perhaps "Accepted" rather than "Acceptable"


================
Comment at: llvm/lib/Support/FileCheck.cpp:233
 
+Expected<ExpressionValue> llvm::max(const ExpressionValue &LeftOperand,
+                                    const ExpressionValue &RightOperand) {
----------------
Should this be an `Expected` if it can't fail? Same for `min`.


================
Comment at: llvm/lib/Support/FileCheck.cpp:255
+                                    const ExpressionValue &RightOperand) {
+  if (cantFail(max(LeftOperand, RightOperand)) == LeftOperand)
+    return RightOperand;
----------------
This `cantFail` call suggests to me that this shouldn't be an `Expected` return.


================
Comment at: llvm/lib/Support/FileCheck.cpp:374-380
+  for (unsigned I = 1, E = Str.size(); I != E; ++I) {
+    // Have we reached the end of the function name of a call expression?
+    if (Str[I] == '(')
+      return true;
+    if (!isValidVarNameChar(Str[I]))
+      break;
+  }
----------------
We should probably allow for optional whitespace between the end of the function name and the `(`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936