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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D79936#2062802 <https://reviews.llvm.org/D79936#2062802>, @paulwalker-arm wrote:

> I'd like to know if it's going to be a requirement to support the reporting of overflow/underflow for the builtins in my patch.  Originally I had gone down the llvm route of making the operations signed rather than the data but I see the signedness patch implements the opposite.  Ultimately I need to know if there's light at the end of the tunnel or whether to give up and just write ugly tests.


(Wrote this comment before I saw you added overflow/underflow support, but leaving it because it might give an idea of my thought process on why): Not quite sure I fully followed this comment. I think my preference would be to error out for overflows/underflows, rather than silently allowing them. If things are going to be significantly more complex adding them but you are also going to address them immediately, I'm okay with it being deferred to a future patch. What I don't want long-term is for people to be able to write unintentionally broken test cases because they happen to be triggering underflow/overflow behaviour. Broken test cases are bad!

In D79936#2064448 <https://reviews.llvm.org/D79936#2064448>, @paulwalker-arm wrote:

> I have a clang-format query.  I'm getting failures because clang-format is suggesting to use "-<space><digit>" to format a negative number.  This doesn't seem correct to me and is not the style I see for existing code in FileCheckTests.cpp.  Is this something I can ignore?


@thopre ran into this recently too. I consider it a bug in clang-format personally, so you can ignore it, but if @thopre hasn't already, you should file a clang-format bug so that it can get fixed.



================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:695
+  A numeric operand is a previously defined numeric variable, an integer
+  literal, or a function call and have a 64-bit precision. The supported
+  operators are ``+`` and ``-``. Spaces are accepted before, after and between
----------------
The "and have a 64-bit precision" bit seems a bit out of place here. It should probably be its own sentence.


================
Comment at: llvm/docs/CommandGuide/FileCheck.rst:696
+  literal, or a function call and have a 64-bit precision. The supported
+  operators are ``+`` and ``-``. Spaces are accepted before, after and between
+  any of these elements. Overflow and underflow are rejected. There is no
----------------
Given the new stuff about functions, I might be tempted to pull out the sentence about supported operators into a list, a bit like that used for the accepted values, especially since it needs updating as things stand!


================
Comment at: llvm/lib/Support/FileCheck.cpp:233
 
+Expected<ExpressionValue> llvm::operator*(const ExpressionValue &LeftOperand,
+                                          const ExpressionValue &RightOperand) {
----------------
I think adding `operator*` etc makes sense, but it should be a separate patch to adding function support (probably a prerequisite). We don't want to cloud the intent of this patch by adding in other useful functionality, and it will make it easier to focus the reviewing.

I'll save reviewing them for that patch.


================
Comment at: llvm/lib/Support/FileCheck.cpp:317-318
+
+Expected<ExpressionValue> llvm::min(const ExpressionValue &LeftOperand,
+                                    const ExpressionValue &RightOperand) {
+  if (LeftOperand.isNegative() && RightOperand.isNegative()) {
----------------
Did you consider writing `min` in terms of `max` (or vice versa)? Not sure if it is a good thing to do or not, but I believe it would lead to less duplicated and more concise code. Something like:

```
if (max(LeftOperand, RightOperand) == LeftOperand)
  return RightOperand;
return LeftOperand;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79936