3

I have a class, for example, Point:

class Point {
public:
    double x, y;
};

And I have some container, for example PointCloud, that wraps inside std::vector<Point*>:

class PointCloud {
public:
   typedef std::vector<Point*>::const_iterator iterator;
   iterator begin();
   iterator end();
private:
  std::vector<Point*> _point
};

Now I can use my class this way:

PointCloud cloud;
for(auto point : cloud) {
  std::cout << point->x << std::endl;
}    

I want to implement begin() and end() so my iterators return references instead of pointers and I could write my code that way:

PointCloud cloud;
for (auto point : cloud) {
  std::cout << point.x << std::endl;
}  

Note: operator . instead of -> operator

I have some reasons to do this thing:

  1. compatibility: Firstly I write my class with std::vector<Point> _point and if I change my mind in the future and replace it with std::vector<Point*> _point and users of my class won't need to change something in their code.

  2. it's just safer to work with references.

  3. There is no need to type -> instead of ..

I work with references instead of pointers.

Is it a right thing to do? An if yes, what would be the best way to implement it?

Finally I found the correct solution: boost::indirect_iterator http://www.boost.org/doc/libs/1_57_0/libs/iterator/doc/indirect_iterator.html

#include <memory>
#include <vector>
#include <iostream>
#include <boost/iterator/indirect_iterator.hpp>

class Point {
public:
    Point(double x, double y) : x(x), y(y) {};
    double x, y;
};

class PointCloud {
public:
    using point_vector = std::vector<std::unique_ptr<Point>>;
    using const_iterator = boost::indirect_iterator<point_vector::const_iterator>;
    const_iterator begin() { return _points.begin(); }
    const_iterator end() { return _points.end(); }
    void insert(double x, double y) { _points.emplace_back(new Point(x, y)); }
private:
    point_vector _points;
};

int main()
{
    PointCloud points;
    points.insert(2, 3);
    points.insert(4, 5);
    for (auto &point : points)
        std::cout << point.x << ' ' << point.y << std::endl;
    return 0;
}
Roman Kolesnikov
  • 11,777
  • 11
  • 44
  • 67
  • 2
    Are you sure you're programming in C++? What's `for each`? What's `in`? When posting questions about code (which should be all questions here on SO) then please make sure to post *valid* code (unless you specifically say it's pseudo-code). – Some programmer dude Dec 26 '14 at 20:38
  • It is a non-standard extension of Visual Studio: http://msdn.microsoft.com/en-us/library/ms177202.aspx – Remy Lebeau Dec 26 '14 at 20:40
  • 3
    Well, I'd start off removing the pointer in the stored entities: `std::vector` should become `std::vector`: it is _much_ more effective to store objects inline (and that's primarily not due to the extra memory allocations). – Dietmar Kühl Dec 26 '14 at 20:40
  • 2
    Also, I would get rid of using any compiler specific (== lock-in) extensions: there seems little reason not to use `for (auto&& point: cloud)` (and, while being pedantic about the code: I'd also replace the costly `std::endl` by using `'\n'`). – Dietmar Kühl Dec 26 '14 at 20:42
  • In the past I had std::vector but now each Point turns to be a huge structure so I need to use pointers – Roman Kolesnikov Dec 26 '14 at 20:43
  • See http://stackoverflow.com/questions/3582608/ – Remy Lebeau Dec 26 '14 at 20:43
  • Using `std::shared_ptr` (or `std::unique_ptr`) to wrap `Point` could be an alternative, depending on usage. – Adam27X Dec 26 '14 at 20:48
  • using smart pointers doesn't solve my problems (points 1-3 at the end of my question) – Roman Kolesnikov Dec 26 '14 at 20:53
  • If you want to return references, how do you think you can represent the end() ? – Jagannath Dec 26 '14 at 20:57
  • I do think about it. So std::vector returns end() iterator. So I think it should be possible to do with references – Roman Kolesnikov Dec 26 '14 at 21:00
  • If the container is empty, what will `end()` return? Remember a reference is a "live" object which has already been constructed. – inetknght Dec 26 '14 at 21:28
  • 1
    Note that using `for (auto point: cloud)` is likely wrong! Although there are cases where it does the Right Thing these are rather rare. The problem is that using `auto point` rather than `auto&& point` (as in my comment above) causes copies of the elements to be made. That is, it is likely a performance problem. Although that is probably not the case for the pointer type you have above it certainly is a problem when iterating over more interesting types. – Dietmar Kühl Dec 26 '14 at 21:34
  • I don't have any solution but I strongly believe any solution would be too much work compared to the changes you would make to reference the underlying object / pointer. – Jagannath Dec 26 '14 at 21:34

2 Answers2

4

Assuming you have access to lvalues you want to expose, a generic deref_iterator<It> is reasonably easy to create although it is a bit of a typing exercise. To cover all iterator categories you'd need to pass through all operations except, obviously, anything accessing the elements, i.e., operators *, ->, and []. In addition to the operations you'll also need to provide the corresponding iterator traits. I don't think there is an easy trick to avoid that.

A corresponding iterator could look something like this (it's not compiled and almost certainly missing some operations):

template <typename It>
class deref_iterator {
    It it;
    using traits     = std::iterator_traits<It>;
    using base_value = typename traits::value_type;
public:
    using value_type        = std::decay_t<decltype(*std::declval<base_value>())>;
    using reference         = value_type&;
    using pointer           = value_type*;
    using difference_type   = typename traits::difference_type;
    using iterator_category = typename traits::iterator_category;

    deref_iterator(): it() {}
    explicit deref_iterator(It it): it(it) {}

    auto operator*() -> reference { return **this->it; }
    auto operator->() -> pointer { return &**this->it; }
    auto operator[](difference_type index) -> reference { return *this->it[index]; }

    auto operator++() -> deref_iterator& { ++this->it; return *this; }
    auto operator++(int) -> deref_iterator { auto rc(*this); ++this->it; return rc; }

    bool operator== (deref_iterator const& other) const { return this->it == other.it; }
    bool operator!= (deref_iterator const& other) const { return !(*this == other); }
    // more in the same spirit for bidirectional and andom access iterator
};

I think the above should be sufficient to deal with forward iteration. Getting more powerful iterators covered is just similar code.

It is worth noting that any muations affecting the sequence won't move the pointers but rather the pointed to values. That is, sequence mutation won't work with such an iterator as the values would get sliced (assuming there are accessible copy operations; if they are not available you'd get a compile-time error). The underlying problem is that STL considers only one object at a given locations while there are conceptually, at least, two:

  • The actual value being accessed.
  • An object holding the value.

For value types these two entities are identical although the one value conceptually still plays two roles. For a sequence like the dereferenced entities the value is the value type (e.g. the Point) while the holding entities is the object pointing to that entities (e.g. the Point*). If these could be distinguished when necessary the STL could be somewhat more powerful.

Dietmar Kühl
  • 150,225
  • 13
  • 225
  • 380
0

If your problem is only for iteration, since you are only talking of looping though and printing.

Why not have two overloads, one taking vector<Point*> and the other vector<Point>.

template<Func&& f>
void LoopThrough(std::vector<Point*> const& v, Func&& f)
{
    for (auto p : v)
    {
        f(p);
    }
}

template<Func&& f>
void LoopThrough(std::vector<Point> const& v, Func&& f)
{
    for (auto p : v)
    {
        f(p);
    }
}

Then keep changing your typedef as per your need.

class PointCloud {
public:
    typedef std::vector<Point*>::const_iterator iterator;
    // typedef std::vector<Point>::const_iterator iterator;
    typedef std::vector<Point*> Vec;
    //typedef std::vector<Point> Vec;

    iterator begin()
    {
        return _point.begin();
    }
    iterator end()
    {
        return _point.end();
    }

    Vec& GetStore()
    {
        return _point;
    }

    const Vec& GetStore() const
    {
        return _point;
    }

    std::vector<Point> _point;
};

you can use it as shown below.

PointCloud p;
p._point.push_back(new Point{ 1.0, 2.0 });
p._point.push_back(new Point{ 3.0, 2.0 });
p._point.push_back(new Point{ 5.0, 2.0 });
p._point.push_back(new Point{ 4.0, 2.0 });


LoopThrough(p._point, [](Point* p) { std::cout << p->x << "\t" << p->y << "\n";  });
Jagannath
  • 3,995
  • 26
  • 30