[PATCH] D23178: [ADT] Add relation operators for Optional

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: unittests/ADT/OptionalTest.cpp:408-411
@@ +407,6 @@
+TEST_F(OptionalTest, OperatorEqual) {
+  //      None 3 4
+  // None    = < <
+  //    3    > = <
+  //    4    > > =
+  CheckOptRelations<EqualTo>(
----------------
dblaikie wrote:
> I don't think we need to test this whole matrix. Looks like for the equality operator we only need 4 cases (each case containing the 5 cases the CheckOptRelations contains): {None, None}, {None, 3}, {3, None}, {3, 3}, {3, 4}). I'm not sure we need more even for the inequality operators, I think we get coverage for operator<, with say: {None, None}, {None, 3}, {3, None}, {3, 4}, {4, 3} - I suppose {3, 3}.
> 
> so, yeah, drop the top right and bottom left corners (because 4 and 3 are equivalent WRT None, but not WRT each other... )
> 
> Hmm, wonder what the best way to describe this is... 
> 
> How would it look if we removed the loops from CheckOptRelations and it just took two Optionals, the relation (via the template parameter), and whether it should be true.
> 
> Then each test should consist of 5 lines that perhaps more clearly show the two operands, the operation, and the truthiness:
> 
>   CheckOptRelations<EqualTo>(None, None, true);
>   CheckOptRelations<EqualTo>(None, Three, false);
>   CheckOptRelations<EqualTo>(Three, None, false);
>   CheckOptRelations<EqualTo>(Three, Three, true);
>   CheckOptRelations<EqualTo>(Three, Four, false);
>   CheckOptRelations<EqualTo>(Four, Three, false); // could skip this one when testing equality/inequality
> 
>  Maybe it can be made more terse (just shorten the name of CheckOptRelations perhaps - it'll be used a lot in a short scope so should be pretty obvious what it does)
> 
> Does any of that seem better/reasonable?
> 
> I suppose alternatively the test utility could take the comparison once, then an initializer_list of {val, val, expected result} tuples (write a little struct to wrap them). Still ensures that the test cases are fairly self explanatory, which comparison is being tested and what result is expected.
> I don't think we need to test this whole matrix. Looks like for the equality operator we only need 4 cases (each case containing the 5 cases the CheckOptRelations contains): {None, None}, {None, 3}, {3, None}, {3, 3}, {3, 4}). I'm not sure we need more even for the inequality operators, I think we get coverage for operator<, with say: {None, None}, {None, 3}, {3, None}, {3, 4}, {4, 3} - I suppose {3, 3}.

Yeah I think 9 cases overlap each other a bit. I can remove duplicated cases, but then we (including the future readers) have to reason about the exhaustiveness of test cases in order to make sure of the coverage, where the 9 cases matrix is so obviously exhaustive.

In exhaustiveness's perspective your suggestion seems less comprehensible.


https://reviews.llvm.org/D23178