Correctly instantiate `iterator_adaptor_base` when defining `pointer_iterator`

[ecstatic-morse]

Summary

The definition of pointer_iterator omits what should be a iterator_traits::<>::iterator_category parameter from iterator_adaptor_base. As a result, iterators based on pointer_iterator always have defaulted value types and the wrong iterator category.

The definition of pointee_iterator just a few lines above does this correctly.

This resolves bug 39617.

[dblaikie]

Could you add a test case? (probably a static_assert in unittests/ADT/iterator.cpp or something like that)

[dblaikie]

(oh, also, looks like this is intended as a fix for PR39617? Good to mention that in reviews/commits - also, there's no need to file a bug before sending a patch. You can skip the bug & send the patch instead if you like!

[ecstatic-morse]

Added tests for pointer_iterator and pointee_iterator to ensure that the category of the underlying iterator is forwarded correctly.

[ecstatic-morse]

I added tests via static_assert and updated the description to mention the bug report.

Sorry, I wasn't sure if I was going to go to the trouble of setting up phabricator when I filed the bug.

[dblaikie]

Looks good - thanks! Do you need me to commit this for you? (Or do you have commit rights & can commit it yourself)

[ecstatic-morse]

I don't have commit rights. Could you commit this for me? I'm new here BTW.

llvm/trunk/include/llvm/ADT/iterator.h

make_pointee_range(RangeT &&Range) {
   using PointeeIteratorT = pointee_iterator<WrappedIteratorT>;
   return make_range(PointeeIteratorT(std::begin(std::forward<RangeT>(Range))),
                     PointeeIteratorT(std::end(std::forward<RangeT>(Range))));
 }
 
 template <typename WrappedIteratorT,
           typename T = decltype(&*std::declval<WrappedIteratorT>())>
 class pointer_iterator
-    : public iterator_adaptor_base<pointer_iterator<WrappedIteratorT, T>,
-                                   WrappedIteratorT, T> {
+    : public iterator_adaptor_base<
+          pointer_iterator<WrappedIteratorT, T>, WrappedIteratorT,
+          typename std::iterator_traits<WrappedIteratorT>::iterator_category,
+          T> {
   mutable T Ptr;
 
 public:
   pointer_iterator() = default;
 
   explicit pointer_iterator(WrappedIteratorT u)
       : pointer_iterator::iterator_adaptor_base(std::move(u)) {}
 

llvm/trunk/unittests/ADT/IteratorTest.cpp

 //===- IteratorTest.cpp - Unit tests for iterator utilities ---------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
+#include "llvm/ADT/ilist.h"
 #include "llvm/ADT/iterator.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
static_assert(std::is_same<typename AdaptedIter::value_type, Shadow<0>>::value,
               "");
 static_assert(
     std::is_same<typename AdaptedIter::difference_type, Shadow<1>>::value, "");
 static_assert(std::is_same<typename AdaptedIter::pointer, Shadow<2>>::value,
               "");
 static_assert(std::is_same<typename AdaptedIter::reference, Shadow<3>>::value,
               "");
 
+// Ensure that pointe{e,r}_iterator adaptors correctly forward the category of
+// the underlying iterator.
+
+using RandomAccessIter = SmallVectorImpl<int*>::iterator;
+using BidiIter = ilist<int*>::iterator;
+
+template<class T>
+using pointee_iterator_defaulted = pointee_iterator<T>;
+template<class T>
+using pointer_iterator_defaulted = pointer_iterator<T>;
+
+// Ensures that an iterator and its adaptation have the same iterator_category.
+template<template<typename> class A, typename It>
+using IsAdaptedIterCategorySame =
+  std::is_same<typename std::iterator_traits<It>::iterator_category,
+               typename std::iterator_traits<A<It>>::iterator_category>;
+
+// pointeE_iterator
+static_assert(IsAdaptedIterCategorySame<pointee_iterator_defaulted,
+                                        RandomAccessIter>::value, "");
+static_assert(IsAdaptedIterCategorySame<pointee_iterator_defaulted,
+                                        BidiIter>::value, "");
+// pointeR_iterator
+static_assert(IsAdaptedIterCategorySame<pointer_iterator_defaulted,
+                                        RandomAccessIter>::value, "");
+static_assert(IsAdaptedIterCategorySame<pointer_iterator_defaulted,
+                                        BidiIter>::value, "");
+
 TEST(PointeeIteratorTest, Basic) {
   int arr[4] = {1, 2, 3, 4};
   SmallVector<int *, 4> V;
   V.push_back(&arr[0]);
   V.push_back(&arr[1]);
   V.push_back(&arr[2]);
   V.push_back(&arr[3]);