[PATCH] D28371: Add missing operators for iterator_facade_base

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: llvm/include/llvm/ADT/iterator.h:117-123
+  bool operator<(const DerivedT &RHS) const {
+    static_assert(
+        IsRandomAccess,
+        "Relational operators are only defined for random access iterators.");
+    return !static_cast<const DerivedT *>(this)->operator>(RHS) &&
+           !static_cast<const DerivedT *>(this)->operator==(RHS);
+  }
----------------
zturner wrote:
> chandlerc wrote:
> > I don't get it...
> > 
> > This recurses through operator>, which recurses through operator<...
> > 
> > The derived class has to define one relational operator. Why can't we insist that it is always operator<? If it is, then the code you've added becomes dead...
> Maybe the problem is just that the expectations of derived classes are not well documented.  It's jarring to be looking through an interface and seeing 3 / 4 relational operators implemented, or not seeing the difference operator implemented and then trying to figure out if it's an oversight in the implementation, or if I'm expected to implement those.  Would it be worth it trying to improve that documentation?
Yea, the set of operations the derived class is supposed to implement was discussed a bunch when this got written and seems to have never made it into comments. =[ Improving them would be fantastic.

Maybe also with pointers to example iterators in the unit tests that actually implement the canonical set of operations for a given iterator category?


https://reviews.llvm.org/D28371