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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:246
+  unsigned RightBitWidth = RightOp.getBitWidth();
+  unsigned NewBitWidth =
+      LeftBitWidth > RightBitWidth ? LeftBitWidth : RightBitWidth;
----------------
std::max?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:252
+    Expected<ExpressionValue> MaybeResult =
+        EvalBinop(ExpressionValue(LeftOp), ExpressionValue(RightOp), Overflow);
+    if (!MaybeResult)
----------------
The API might be a bit nicer if evalbinop did the widening of apints?


================
Comment at: llvm/lib/FileCheck/FileCheck.cpp:263
+  } while (true);
+  llvm_unreachable("Unexpected loop break");
 }
----------------
This should be unnecessary.


================
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).
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150880