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

[David Blaikie via llvm-commits]

dblaikie added a comment.

Still trying to figure out how to make the test cases both short and legible - open to ideas, there's no need to take my suggestions as decree or anything.


================
Comment at: unittests/ADT/OptionalTest.cpp:388-396
@@ +387,11 @@
+
+      if (Opts[i])
+        EXPECT_EQ(Results[i][j], OperatorT::apply(*Opts[i], Opts[j]));
+      else
+        EXPECT_EQ(Results[i][j], OperatorT::apply(None, Opts[j]));
+
+      if (Opts[j])
+        EXPECT_EQ(Results[i][j], OperatorT::apply(Opts[i], *Opts[j]));
+      else
+        EXPECT_EQ(Results[i][j], OperatorT::apply(Opts[i], None));
+    }
----------------
Nice way to get the extra coverage over the asymmetric comparisons!

================
Comment at: unittests/ADT/OptionalTest.cpp:408-411
@@ +407,6 @@
+TEST_F(OptionalTest, OperatorEqual) {
+  //      None 3 4
+  // None    = < <
+  //    3    > = <
+  //    4    > > =
+  CheckOptRelations<EqualTo>(
----------------
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.


https://reviews.llvm.org/D23178