2

I've copied some code from a C++ cookbook to implement a strided iterator. The iterator seems to work with other stl functions like copy, but won't work with sort. My guess is that it has something to do with there being some operators missing. Here's my header file for the strided iterator (From the Oreilly C++ cookbook)

#define STRIDEITER_HPP

#include <iterator>
#include <cassert>

template<class Iter_T>
class stride_iter
{
public:
  // public typedefs
  typedef typename std::iterator_traits<Iter_T>::value_type value_type;
  typedef typename std::iterator_traits<Iter_T>::reference reference;
  typedef typename std::iterator_traits<Iter_T>::difference_type difference_type;
  typedef typename std::iterator_traits<Iter_T>::pointer pointer;
  typedef std::random_access_iterator_tag iterator_category;
  typedef stride_iter self;

  // constructors
  stride_iter( ) : m(NULL), step(0) { };
  stride_iter(const self& x) : m(x.m), step(x.step) { }
  stride_iter(Iter_T x, difference_type n) : m(x), step(n) { }

  // operators
  self& operator++( ) { m += step; return *this; }
  self operator++(int) { self tmp = *this; m += step; return tmp; }
  self& operator+=(const difference_type x) { m += (x * step); return *this; }
  self& operator--( ) { m -= step; return *this; }
  self operator--(int) { self tmp = *this; m -= step; return tmp; }
  self& operator-=(const difference_type x) { m -= x * step; return *this; }
  reference operator[](const difference_type n) { return m[n * step]; }
  reference operator*( ) { return *m; }

  // friend operators
  friend bool operator==(const self& x, const self& y) {
    assert(x.step == y.step);
    return x.m == y.m;
  }
  friend bool operator!=(const self& x, const self& y) {
    assert(x.step == y.step);
    return x.m != y.m;
  }
  friend bool operator<(const self& x, const self& y) {
    assert(x.step == y.step);
    return x.m < y.m;
  }
  friend difference_type operator-(const self& x, const self& y) {
    assert(x.step == y.step);
    return (x.m - y.m) / x.step;
  }

  friend self operator+(const self& x, difference_type y) {
    assert(x.step == y.step);
    return x += (y * x.step);
  }
  friend self operator+(difference_type x, const self& y) {
    assert(x.step == y.step);
    return y += x * x.step;
  }
private:
  Iter_T m;
  difference_type step;
};
#endif

I call is using 

#include "strideiter.hpp"
#include <algorithm>
#include <iterator>
#include <iostream>

using namespace std;

int main( ) {

  int *a;
  a =(int*) malloc(10*sizeof(int));

  for(int i=0; i<10; i++)
    {
      a[i]=10-i;
    }

  int skip=2;

  stride_iter<int*> first(a+2, skip);
  stride_iter<int*> last(a + 2+8, skip);
  sort(first,last);
}

I get several errors, the first one is:

strideiter.hpp(52): error: expression must have class type
  assert(x.step == y.step);

Do I need multiple implementations for +=?

nwknoblauch
  • 548
  • 4
  • 12
  • 4
    `y` is of type `difference_type`, which can be/is an integer like `int` or `long` and therefore has no members (`y.step`). – dyp Sep 26 '13 at 17:02
  • `a =(int*) malloc(10*sizeof(int));` Why don't you use the C++ tools? `std::array a;`, `std::vector a(10);` or even `int *a = new int[10];`, although `std::unique_ptr a{new int[10]};` would be slightly better. – dyp Sep 26 '13 at 17:09
  • Because `malloc` is really `mkl_malloc` for aligned memory, and it's going to be used for some very large matrix multiplication operations that only work on plain old arrays. – nwknoblauch Sep 26 '13 at 17:13
  • Ok, that sounds much more reasonable than plain `malloc`, even though I'd still recommend using smart pointers (e.g. a `unique_ptr` with a custom deleter). – dyp Sep 26 '13 at 17:17
  • As James Kanze pointed out (or alluded to) you already have undefined behaviour here: `a + 2+8`. As you're already working with the non-C++-standard `mkl_malloc`, make sure your implementation behaves "as expected". – dyp Sep 26 '13 at 17:33

2 Answers2

4

Fixing operator+ and providing an operator-(self, difference_type) and it compiles fine. The RandomAccessIterator "concept" and std::sort have a vast amount of requirements, which you can find here in another question involving iterators.

friend self operator+(const self& x, difference_type y) {
  // do not modify `x`, but return a modified copy
  return self(x.m + (y * x.step), x.step);
  // idiomatically:
  // self temp(x);
  // temp += y;
  // return temp;
}
friend self operator+(difference_type x, const self& y) {
  return y+x;
}

friend self operator-(const self& x, difference_type y) {
  return self(x.m - (y * x.step), x.step);
}

For this usage:

int main( ) {

  int a[10];
  //a =(int*) malloc(10*sizeof(int));

  for(int i=0; i<10; i++)
    {
      a[i]=10-i;
    }

  for(int e : a) std::cout << e << ", ";
  std::cout << std::endl;

  int skip=2;

  stride_iter<int*> first(a+2, skip);
  stride_iter<int*> last(a + 2+8, skip);
  sort(first,last);

  for(int e : a) std::cout << e << ", ";
  std::cout << std::endl;
}

The output is:

10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 
10, 9, 2, 7, 4, 5, 6, 3, 8, 1,

Which seems reasonable to me.

Community
  • 1
  • 1
dyp
  • 38,334
  • 13
  • 112
  • 177
1

The problem is that you're declaring the iterator to be a random_access_iterator, but you're not providing the operator+ and operator- functions.

Having said that, your iterator is playing with fire, since it will only work if the size of the container is an exact multiple of the stride. If you have a container with, say, 100 elements, and you ask for a stride of 3, you'll have undefined behavior. To be useful, your iterator will have to pick up both the start and end of the range, so that it can avoid striding over the end (or the beginning, when striding backwards).

James Kanze
  • 150,581
  • 18
  • 184
  • 329
  • "Having said that, your iterator is playing with fire, since it will only work if the size of the container is an exact multiple of the stride" I thought the same, but the burden isn't entirely on the iterator. As shown in the example, the creator of the iterator can also check if the iterator can be safely used for a purpose (read: the end iterator). A past-the-end iterator isn't always possible, though, I agree. – dyp Sep 26 '13 at 17:19
  • @DyP The problem is that the constraint `size % stride == 0` isn't reasonable. So when you're advancing, you have to check whether you can advance or not, and if not, advance as far as you can (which will put you one past the end). – James Kanze Sep 26 '13 at 17:23
  • What I was referring to is that the size of the *range* implicitly defined via begin and end iterator (and not the size of the container) is required to have `size%stride==0`. This requirement has to/can be checked by the creator of the range, but cannot be reasonably be checked by the iterator itself (e.g. offsets). A range library might be better suited for striding iterators. – dyp Sep 26 '13 at 17:28
  • @DyP I understood what you meant, but... it makes it impossible to visit the last element that should be visited in a container, because the end iterator for the range would be beyond the end of the container. – James Kanze Sep 26 '13 at 18:09
  • I agree, at least not w/o UB. The OP is already using a non-standard allocation function (`mkl_malloc`, see comments), so possibly the behaviour is well-defined for the implementation. Without relying on UB/implementation-defined behaviour, the iterator necessarily gets bigger and more complex. – dyp Sep 26 '13 at 19:10