[PATCH] D95803: Ensure that InstructionCost actually implements a total ordering

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/unittests/Support/InstructionCostTest.cpp:25
+
+  ASSERT_TRUE(DefaultCost.isValid());
+  EXPECT_EQ(*(DefaultCost.getValue()), 0);
----------------
sdesmalen wrote:
> is there a reason you're preferring ASSERT over EXPECT in this test, and not the one below (getValue() == 0). I would think EXPECT is sufficient.
if the DefaultCost is invalid, then the dereference of the getValue below will cause a crash, preventing the rest of the tests from running.

If I change the default to Invalid then run the test:

with `ASSERT_TRUE`:

```
[----------] 2 tests from CostTest
[ RUN      ] CostTest.DefaultCtor
[redacted]\llvm\unittests\Support\InstructionCostTest.cpp(25): error: Value of: DefaultCost.isValid()
  Actual: false
Expected: true
[  FAILED  ] CostTest.DefaultCtor (1 ms)
[ RUN      ] CostTest.Operators
[       OK ] CostTest.Operators (0 ms)
[----------] 2 tests from CostTest (3 ms total)
```

with `EXPECT_TRUE`:
```
[----------] 2 tests from CostTest
[ RUN      ] CostTest.DefaultCtor
[redacted]\llvm\unittests\Support\InstructionCostTest.cpp(25): error: Value of: DefaultCost.isValid()
  Actual: false
Expected: true
Assertion failed: hasVal, file [redacted]\llvm\include\llvm/ADT/Optional.h, line 197
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95803