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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm marked 3 inline comments as done.
paulwalker-arm added a comment.





================
Comment at: llvm/lib/Support/FileCheck.cpp:233
 
+Expected<ExpressionValue> llvm::max(const ExpressionValue &LeftOperand,
+                                    const ExpressionValue &RightOperand) {
----------------
jhenderson wrote:
> Should this be an `Expected` if it can't fail? Same for `min`.
This is to match the binop_eval_t typedef required by BinaryOperation.


================
Comment at: llvm/lib/Support/FileCheck.cpp:255
+                                    const ExpressionValue &RightOperand) {
+  if (cantFail(max(LeftOperand, RightOperand)) == LeftOperand)
+    return RightOperand;
----------------
jhenderson wrote:
> This `cantFail` call suggests to me that this shouldn't be an `Expected` return.
As explained above. There did not seem much value in creating a duplicate set of functions (i.e. with and without an Expected result) given this is the only other use.


================
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;
+  }
----------------
jhenderson wrote:
> We should probably allow for optional whitespace between the end of the function name and the `(`.
In LLVM's c/c++ world clang-format will remove such whitespace (presumably to aid readability) so do we really want to allow it in FileCheck code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936