[PATCH] The iterators in CFG.h are not conforming and thus causing problem

Howard Hinnant hhinnant at apple.com
Wed Mar 27 10:14:37 PDT 2013


This is a minor fix.  Please review.

Seciton 24.2.2 of the C++ standard, [iterator.iterators], Table 106 requires that the return type of *r for all iterators r be reference, where reference is defined in [iterator.requirements.general]/p11 as iterator_traits<X>::reference, and X is the type of r.

But in CFG.h, the dereference operator of PredIterator and SuccIterator return pointer, not reference.

Furthermore the nested type reference is value_type&, which is not the type returned from operator*().

This patch simply makes the iterator type value_type*, which is what the operator*() returns, and then re-lables the return type as reference.

>From a functionality point of view, the only difference is that the nested reference type is now value_type* instead of value_type&.

---------------------------------

This change is important because of a recent (but failed) change in libc++, r178075:

--- libcxx/trunk/include/vector (original)
+++ libcxx/trunk/include/vector Tue Mar 26 16:40:54 2013
@@ -526,17 +526,29 @@ public:
    template <class _InputIterator>
        vector(_InputIterator __first, _InputIterator __last,
               typename enable_if<__is_input_iterator  <_InputIterator>::value &&
-                                 !__is_forward_iterator<_InputIterator>::value>::type* = 0);
+                                 !__is_forward_iterator<_InputIterator>::value &&
+                                 is_constructible<
+                                    value_type,
+                                    typename iterator_traits<_InputIterator>::reference>::value>::type* = 0);

This concerns the member template constructor of vector:
 
template <class InputIterator>
  vector(InputIterator first, InputIterator last, const Allocator& = Allocator());

[sequence.reqmts]/p14/p15 says:

> ... are called with a type InputIterator that does not qualify as an input iterator, then these functions shall not participate in overload resolution.

> The extent to which an implementation determines that a type cannot be an input iterator is unspecified, except that as a minimum integral types shall not qualify as input iterators.


Prior to r178075 libc++ used a trait called __is_input_iterator<InputIterator>::value to detect whether InputIterator is a real iterator or not.  It works quite well except that this trait does not say what type the InputIterator is supposed to refer to.

Someone on Stack Overflow rightly pointed out that if the InputIterator referred to the wrong value_type that is_constructible<vector<T>>, InputIterator, InputIterator>::value would mistakenly answer true.  r178075 corrects this by adding a constraint that the vector's value_type must be constructible from the dereferenced InputIterator (aka iterator_traits<InputIterator>::reference). 

With this change, is_constructible<vector<T>>, InputIterator, InputIterator>::value always gives the correct answer.

But it broke the non-conforming iterators in CFG.h, and thus Daniel Dunbar rightly reverted the libc++ commit.

Please apply this llvm patch so that the quality of libc++ can be kicked up a notch.

Thanks,
Howard

---------------------------------

Files:
  include/llvm/Support/CFG.h

Index: include/llvm/Support/CFG.h
===================================================================
--- include/llvm/Support/CFG.h	(revision 178146)
+++ include/llvm/Support/CFG.h	(working copy)
@@ -27,8 +27,9 @@
 
 template <class Ptr, class USE_iterator> // Predecessor Iterator
 class PredIterator : public std::iterator<std::forward_iterator_tag,
-                                          Ptr, ptrdiff_t> {
-  typedef std::iterator<std::forward_iterator_tag, Ptr, ptrdiff_t> super;
+                                          Ptr, ptrdiff_t, Ptr*, Ptr*> {
+  typedef std::iterator<std::forward_iterator_tag, Ptr, ptrdiff_t, Ptr*,
+                                                                    Ptr*> super;
   typedef PredIterator<Ptr, USE_iterator> Self;
   USE_iterator It;
 
@@ -40,6 +41,7 @@
 
 public:
   typedef typename super::pointer pointer;
+  typedef typename super::reference reference;
 
   PredIterator() {}
   explicit inline PredIterator(Ptr *bb) : It(bb->use_begin()) {
@@ -50,7 +52,7 @@
   inline bool operator==(const Self& x) const { return It == x.It; }
   inline bool operator!=(const Self& x) const { return !operator==(x); }
 
-  inline pointer operator*() const {
+  inline reference operator*() const {
     assert(!It.atEnd() && "pred_iterator out of range!");
     return cast<TerminatorInst>(*It)->getParent();
   }
@@ -100,10 +102,11 @@
 
 template <class Term_, class BB_>           // Successor Iterator
 class SuccIterator : public std::iterator<std::bidirectional_iterator_tag,
-                                          BB_, ptrdiff_t> {
+                                          BB_, ptrdiff_t, BB_*, BB_*> {
   const Term_ Term;
   unsigned idx;
-  typedef std::iterator<std::bidirectional_iterator_tag, BB_, ptrdiff_t> super;
+  typedef std::iterator<std::bidirectional_iterator_tag, BB_, ptrdiff_t, BB_*,
+                                                                    BB_*> super;
   typedef SuccIterator<Term_, BB_> Self;
 
   inline bool index_is_valid(int idx) {
@@ -112,6 +115,7 @@
 
 public:
   typedef typename super::pointer pointer;
+  typedef typename super::reference reference;
   // TODO: This can be random access iterator, only operator[] missing.
 
   explicit inline SuccIterator(Term_ T) : Term(T), idx(0) {// begin iterator
@@ -142,7 +146,7 @@
   inline bool operator==(const Self& x) const { return idx == x.idx; }
   inline bool operator!=(const Self& x) const { return !operator==(x); }
 
-  inline pointer operator*() const { return Term->getSuccessor(idx); }
+  inline reference operator*() const { return Term->getSuccessor(idx); }
   inline pointer operator->() const { return operator*(); }
 
   inline Self& operator++() { ++idx; return *this; } // Preincrement
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CFG.h.patch
Type: application/octet-stream
Size: 2746 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130327/a5cb5721/attachment.obj>


More information about the llvm-commits mailing list