[PATCH] D28371: Add missing operators for iterator_facade_base

[Zachary Turner via Phabricator via llvm-commits]

zturner 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);
+  }
----------------
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?


https://reviews.llvm.org/D28371