[PATCH] D29619: ADT: Add explicit conversions for reverse ilist iterators

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Seems (to my best understanding) this is necessary (there must be an iterator->reverse_iterator ctor that does the off-by-1 thing) and sufficient (it can/must be explicit) to meet the C++ standard "reversible container" requirements. (I was curious if 'explicit' was OK, seems it may be necessary - so, great!)

(some optional feedback on the testing - wrote it about one of the test cases, applies equally to the other)



================
Comment at: llvm/unittests/ADT/IListIteratorTest.cpp:147-165
+  // Check explicit conversions.
+  EXPECT_EQ(++CL.begin(), const_iterator(++CL.rbegin()));
+  EXPECT_EQ(++CL.begin(), const_iterator(++L.rbegin()));
+  EXPECT_EQ(++CL.rbegin(), const_reverse_iterator(++CL.begin()));
+  EXPECT_EQ(++CL.rbegin(), const_reverse_iterator(++L.begin()));
+  EXPECT_EQ(++L.begin(), iterator(++L.rbegin()));
+  EXPECT_EQ(++L.rbegin(), reverse_iterator(++L.begin()));
----------------
Is it necessary to test all these dimensions together? (I take it this is testing all conversions to/from reverse iterators, const (reverse) iterators, and begin/end, and one off begin, so that's 2 * 2 * 2 * 3.

Are some of these in useful equivalence classes that could be reduced down? (for example the const V non-const seems pretty orthogonal to the value set (begin/end/begin+1) - actually they all seem pretty orthogonal & so perhaps 2 + 2 + 2 + 3 test cases would suffice (or maybe even less if some of the tests cross equivalence classes)?)


================
Comment at: llvm/unittests/ADT/IListIteratorTest.cpp:168-173
+  EXPECT_FALSE((std::is_convertible<iterator, reverse_iterator>::value));
+  EXPECT_FALSE((std::is_convertible<reverse_iterator, iterator>::value));
+  EXPECT_FALSE(
+      (std::is_convertible<const_iterator, const_reverse_iterator>::value));
+  EXPECT_FALSE(
+      (std::is_convertible<const_reverse_iterator, const_iterator>::value));
----------------
static_assert instead?


https://reviews.llvm.org/D29619