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

[David Blaikie via llvm-commits]

dblaikie 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>(
----------------
timshen wrote:
> 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.
For myself it raised the opposite question "what's all this testing - are there cases/complexities in the code that I hadn't seen"/"why are there so many 'interesting' cases here"?

Hmm, maybe we could simplify it a bit if we created a noisy type - ie: we shouldn't need to test {3, 3}, {3, 4}, etc... - that's just the underlying type's operator. If we test that it calls the underlying type's operator when both values are non-empty, that should be sufficient. Then we can test the None cases.

That'd still be all the 3 variants of each of {None, None}, {None, Something}, {Something, None}, {Something, Something}.

Happy to chat in person/IRC/pair program up some possible ways this might look - might be faster. (& maybe there's an existing similar ADT in LLVM & we could see how it tests such a matrix? (but my first few attempts to look - APInt/Float didn't find anything useful to draw a parallel from))


https://reviews.llvm.org/D23178