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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/unittests/Support/InstructionCostTest.cpp:25
+
+  ASSERT_TRUE(DefaultCost.isValid());
+  EXPECT_EQ(*(DefaultCost.getValue()), 0);
----------------
ctetreau wrote:
> 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
> ```
Thanks for explaining, I didn't realise that was how ASSERT_TRUE worked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95803