[PATCH] D60388: FileCheck [8/12]: Define numeric var from expr

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

thopre added inline comments.


================
Comment at: llvm/lib/Support/FileCheck.cpp:37
+  if (ExpressionAST != nullptr) {
+    Expected<uint64_t> EvaluatedValue = ExpressionAST->eval();
+    assert(EvaluatedValue &&
----------------
jhenderson wrote:
> thopre wrote:
> > jhenderson wrote:
> > > This is unchecked in builds without assertions.
> > That's fine. setValue is called after substitution has happened and match was successful. Therefore ExpressionAST ought to evaluate fine. This is just in case code is reworked in the future and this invariant is broken.
> It isn't. LLVM Errors and Expecteds HAVE to be checked. If they aren't it will lead to an abort. From the Programmer's Manual:
> 
> > All Error instances, whether success or failure, must be either checked or moved from (via std::move or a return) before they are destructed. Accidentally discarding an unchecked error will cause a program abort at the point where the unchecked value’s destructor is run, making it easy to identify and fix violations of this rule.
> 
> 
Oh my bad, didn't make the connection with the Expected type. I think an abort would be fine in this case since that error should never occur in any of the executions. By that I mean that code path should be dead (that function will not be called if the expression does not evaluate). If it does, it's a bug somewhere in the code.

Does that make sense? I've added some comments on the expectation for the caller both in the declaration and just on top of the assert since that was missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60388