Iterator design pattern using unique_ptr instead of raw ptr

Question

I've got this fairly simple C++ code from refactoring.guru. It compiles and works as expected. However, it uses new and delete via the CreateIterator() member function, and I'd like to port it to use unique_ptr, but I'm not sure how to do that exactly.

I've created a CreateIterator2() function using unique_ptr, which compiles (I think this is what I want), but not if I invoke it, so I commented out the invocation.

I'm not sure what I am missing to return a smart pointer and avoid using raw new/delete.

// refactoring.guru c++ design patterns
#include <iostream>
#include <string>
#include <vector>
#include <memory>

template<typename T, typename U>
class Iterator {
public:
    typedef typename std::vector<T>::iterator iter_type;

    Iterator(U* p_data, bool reverse = false) : m_p_data_(p_data) {
        m_it_ = m_p_data_->m_data_.begin();
    }

    void First() {
        m_it_ = m_p_data_->m_data_.begin();
    }

    void Next() {
        m_it_++;
    }

    bool IsDone() {
        return (m_it_ == m_p_data_->m_data_.end());
    }

    iter_type Current() {
        return m_it_;
    }

private:
    U* m_p_data_;
    iter_type m_it_;
};

template<class T>
class Container {
    friend class Iterator<T, Container>;

public:
    void Add(T a) {
        m_data_.push_back(a);
    }

    Iterator<T, Container>* CreateIterator() {
        // want to avoid this
        return new Iterator<T, Container>(this);
    }

    //think this is what I want  - compiles
    std::unique_ptr<Iterator<T, Container>> CreateIterator2() {
        return std::unique_ptr<Iterator<T, Container>>(this);
    }

private:
    std::vector<T> m_data_;
};

class Data {
public:
    Data(int a = 0) : m_data_(a) {}

    void set_data(int a) {
        m_data_ = a;
    }

    int data() {
        return m_data_;
}

private:
    int m_data_;
};

void ClientCode() {
    std::cout << "________________Iterator with int______________________________________" <<        std::endl;
    Container<int> cont;

    for (int i = 0; i < 10; i++) {
        cont.Add(i);
    }

    Iterator<int, Container<int>>* it = cont.CreateIterator();
    for (it->First(); !it->IsDone(); it->Next()) {
        std::cout << *it->Current() << std::endl;
    }

    //THIS DOESN"T COMPILE
    //std::unique_ptr<Iterator<int, Container<int>> it = cont.CreateIterator2();
    //for (it->First(); !it->IsDone(); it->Next()) {
    //    std::cout << *it->Current() << std::endl;
    //}


    Container<Data> cont2;
    Data a(100), b(1000), c(10000);
    cont2.Add(a);
    cont2.Add(b);
    cont2.Add(c);

    std::cout << "________________Iterator with custom Class______________________________" << std::endl;
    Iterator<Data, Container<Data>>* it2 = cont2.CreateIterator();
    for (it2->First(); !it2->IsDone(); it2->Next()) {
    std::cout << it2->Current()->data() << std::endl;
    }
    //avoid this
    delete it;
    delete it2;
}

int main() {
    ClientCode();
    return 0;
}

Answer 1

By default, std::unique_ptr simply wraps a new'ed object, and its destructor will call delete on that object.

So, this statement:

return std::unique_ptr<Iterator<T, Container>>(this);

is just plain wrong, as this is not a new'd object that unique_ptr can take ownership of. This statement is NOT equivalent to the original statement:

return new Iterator<T, Container>(this);

The original constructs a new Iterator object with this being passed in as an input parameter to its constructor. But your replacement does not even construct an Iterator object at all.

The correct equivalent statement is:

return std::unique_ptr<Iterator<T, Container>>(new Iterator<T, Container>(this));

Or better:

return std::make_unique<Iterator<T, Container>>(this);