[PATCH] D150880: [RFC, FileCheck, 3/4] Allow AP value for numeric expressions

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

thopre added inline comments.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:252
+    Expected<ExpressionValue> MaybeResult =
+        EvalBinop(ExpressionValue(LeftOp), ExpressionValue(RightOp), Overflow);
+    if (!MaybeResult)
----------------
arichardson wrote:
> The API might be a bit nicer if evalbinop did the widening of apints?
EvalBinop is a function pointer to one of the expr* functions above. That would mean adding that logic in all those functions. This code here is in a single place instead. I could split it out into a separate function if you prefer?


================
Comment at: llvm/test/FileCheck/match-time-error-propagation/matched-excluded-pattern.txt:4
-;
-; At one time, FileCheck's exit status for the following example was zero even
-; though the excluded pattern does match (it's just capturing that fails).
----------------
arichardson wrote:
> thopre wrote:
> > arichardson wrote:
> > > It seems like some of this test is still useful, we just have to create a different error?
> > This patch removes the one case where the error path tested by these tests can happen. After this patch the parsing of integer in valueFromStringRepr() would need to fail for the path to be triggered by the regex used to match numeric values ensures that integer are valid. See https://reviews.llvm.org/D154431 for more details.
> Ah fair enough that makes sense. Is the error handling still tested after removing this test? If not, could keep coverage by changing the error to be a divide by zero?
TL;DR: the code tested by this is dead code, it cannot be exercise.

Long answer:
Divide by zero can only happens when substituting a numeric expression which leads to a match failure. These tests were for errors after a successful match, which only happened when a value could not fit in a numeric variable due to overflow/underflow. As can be seen by the removal of the code in match() in [[ https://reviews.llvm.org/D154431 | D154431 ]] there is no more any failure in match() after a successful match. The whole code that this test was testing is effectively dead and could be removed, but I'm wary of removing it in case some change in FileCheck makes it necessary again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150880