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

Fri May 29 01:35:55 PDT 2020

jhenderson added a comment.

Some of my testing suggestions might better belong in the unit tests. Also, you're probably going to need to rebase and expand the behaviour somewhat now that signed values support has landed in D60390 <https://reviews.llvm.org/D60390>.

================
Comment at: llvm/lib/Support/FileCheck.cpp:287
+  // Try to parse a function call.
+  if (Expr.startswith(CallPrefix)) {
+    if (AO != AllowedOperand::Any)
----------------
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.

================
Comment at: llvm/lib/Support/FileCheck.cpp:436-437
+  StringRef FuncName = Expr.take_front(FuncNameEnd);
+  if (FuncName.empty())
+    return ErrorDiagnostic::get(SM, Expr, "missing function name");
+
----------------
I believe you could change this case to an asseertion - the parseExpr function treats a leading '(' as a different kind of expression.

================
Comment at: llvm/lib/Support/FileCheck.cpp:521
     Expr = Expr.ltrim(SpaceChars);
     if (!Expr.consume_front("%"))
       return ErrorDiagnostic::get(
----------------
In conjunction with my suggestion above about not having a function specifier, you could change this code to bail out without error in some cases, perhaps by starting with looking for the `%` followed by some specific characters, followed by a `,`.

================
Comment at: llvm/lib/Support/FileCheckImpl.h:663
   /// Parses \p Expr for use of a numeric operand at line \p LineNumber, or
   /// before input is parsed if \p LineNumber is None. Accepts both literal
+  /// values, numeric variables and function calls, depending on the value of
----------------
I think you need to delete "both" here, since there are now three different things it accepts.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:117
+22  // CHECK-NEXT: {{^}}[[# %u , !mul(UNSI,2) ]]
+67  // CHECK-NEXT: {{^}}[[# %u , UNSI + !udiv( !mul(100 , UNSI ), 20) +1 ]]

----------------
Perhaps change the inner `UNSI` to `UNSI+1` or something to show that the argument of a function is any kind of expression? Up to you.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:378
+RUN: not FileCheck -D#NUMVAR=10 --check-prefix CALL-MISSING-CLOSING-BRACKET --input-file %s %s 2>&1 \
+RUN:   | FileCheck --strict-whitespace --check-prefix CALL-MISSING-CLOSING-BRACKET-MSG %s
+RUN: %ProtectFileCheckOutput \
----------------
I would prefer these to be interleaved with their corresponding CHECK and input text:

```
RUN: ... --check-prefix CALL-MISSING-OPENING-BRACKET ...

CALL MISSING OPENING BRACKET
30
CALL-MISSING-OPENING-BRACKET-LABEL: ...
...

RUN: ... --check-prefix CALL-MISSING-CLOSING-BRACKET ...

CALL MISSING CLOSING BRACKET
```
etc. It helps reduce the distance I have to look to find the thing being checked for.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:403
+CALL-MISSING-CLOSING-BRACKET-LABEL: CALL MISSING CLOSING BRACKET
+CALL-MISSING-CLOSING-BRACKET-NEXT: [[#!mul(NUMVAR,3]]
+CALL-MISSING-CLOSING-BRACKET-MSG: numeric-expression.txt:[[#@LINE-1]]:52: error: missing ')' at end of call expression
----------------
There might want to be some interaction testing with plain parentheses. Something like `[[#!mul(NUMVAR,(NUMVAR+3))]]` and `[[#!mul(NUMVAR,(NUMVAR+3)]]` (the first should work, but not the second).

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:411
+CALL-MISSING-NAME-LABEL: CALL MISSING NAME
+CALL-MISSING-NAME-NEXT: [[#!(NUMVAR)]]
+CALL-MISSING-NAME-MSG: numeric-expression.txt:[[#@LINE-1]]:29: error: missing function name
----------------
Nit: it would probably be best to make this call take two arguments.

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:419
+CALL-MISSING-ARGUMENT-LABEL: CALL MISSING ARGUMENT
+CALL-MISSING-ARGUMENT-NEXT: [[#!mul(NUMVAR,)]]
+CALL-MISSING-ARGUMENT-MSG: numeric-expression.txt:[[#@LINE-1]]:44: error: missing argument
----------------
I think you also want the following: `[[#!mul(,NUMVAR)]]`

Possibly also `[[#!mul(NUMVAR,,NUMVAR)]]`

================
Comment at: llvm/test/FileCheck/numeric-expression.txt:435
+CALL-UNDEFINED-FUNCTION-LABEL: CALL UNDEFINED FUNCTION
+CALL-UNDEFINED-FUNCTION-NEXT: [[#!bogus_function(NUMVAR)]]
+CALL-UNDEFINED-FUNCTION-MSG: numeric-expression.txt:[[#@LINE-1]]:35: error: call to undefined function 'bogus_function'
----------------
Nit: it would probably be best to make this call take two arguments.

================
Comment at: llvm/unittests/Support/FileCheckTest.cpp:784
+                        Tester.parseSubst("!(FOO)").takeError());
+  expectDiagnosticError("missing argument",
+                        Tester.parseSubst("!mul(FOO,)").takeError());
----------------
arichardson wrote:
> Might make sense to add case with missing operators such as `2!mul(FOO,2)` or `FOO !mul(FOO,2)` or `!mul(FOO(!mul(3,2)))`
+1

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936