Dereferencing not working for smart pointer in set

Question

#include<iostream>
#include<set>
template <typename T>
/* Simple smart pointer class */
class SmartPtr
{
    T *ptr;
public:
    explicit SmartPtr(T *p = NULL) { ptr = p; }
    ~SmartPtr() { delete(ptr); }
    T & operator * () { return *ptr; }

    T * operator -> () { return ptr; }

};

class simple {

private:
    int x;
public:
    simple(int y = 0) :x(y) {}
    int getX() { return x; }
};
typedef SmartPtr<simple> simplePtr;



int main() {


    std::set<simplePtr> st;
    simplePtr p1 = simplePtr(new simple(5));
    simplePtr p2 = simplePtr(new simple(5));
    simplePtr p3 = simplePtr(new simple(5));
    simplePtr p4 = simplePtr(new simple(5));

    std::cout << p1->getX();  <-- working fine
    st.insert(p1);
    st.insert(p2);
    st.insert(p3);
    st.insert(p4);

    for (std::set<simplePtr>::iterator it = st.begin(); it != st.end(); ++it)
    {
        std::cout << it->getX();  // Not working??
    }
}

Compilation is failed with error in Visual Studio 2013:

Error   C2039   getX: is not a member of SmartPtr<simple>

On linux:

error: ‘const class SmartPtr<simple>’ has no member named ‘getX’

Is this a problem with iterator??

Answer 1

You can think of it->getX() as a syntactic sugar for (*it).getX(). [In principle, a class can overload the -> and * (dereferencing) operators inconsistently, but std::set<T>::iterator, unsurprisingly, doesn't break that convention]. So, in your case, *it is dereferenced to an lvalue of type const SmartPtr<simple>&, and the .getX() applied to it fails, because SmartPtr doesn't have a getX() method. Since, instead you mean to access the object that the obtained SmartPtr points to, you must add one more level of dereferencing:

Correction 1

Replace it->getX() with (**it).getX() or (*it)->getX().

There is still another problem, though - *it results in a const SmartPtr (yes, std::set's non-constant iterator doesn't provide write access to the container's elements, otherwise you could break correct ordering of elements in the container). But both -> and * (dereferencing) operators in SmartPtr are defined in such a way that they can be invoked only on non-const objects. To fix that, you must make those two functions const:

Correction 2 (in SmartPtr<T>)

//                 vvvvv
T & operator * ()  const { return *ptr; }
T * operator -> () const { return ptr; }
//                 ^^^^^

After you make this second correction, you can replace your old-style for-loop with a range-for loop:

for (const simplePtr& p : st)
{
    std::cout << p->getX();
}

Still, your program will not compile - SmartPtr<T> objects cannot be put in an std::set since they are not comparable. Fix that by defining operator<():

Correction 3

Add to SmartPtr<T>:

bool operator<(const SmartPtr& other) const { return ptr < other.ptr; }

At this point your code will compile but chances are high that it will not work correctly. The reason is that the copy-semantics of SmartPtr<T> is left to compiler's discretion which fails to meet your intent. This is easy to guess by spotting the violation of the Rule of Three, Four and Five - your class defines the destructor but fails to define the copy and/or move constructor and the assignment operator. As a result your code performs double deletion and therefore cannot be guaranteed any well defined behavior.

Correction 4

Fix the copy semantics of SmartPtr<T>.

I "fixed" your code by assigning move semantics to SmartPtr (this required adding std::move() when insert()-ing it into std::set):

#include<iostream>
#include<set>

template <typename T>
class SmartPtr
{
    T *ptr;
public:
    explicit SmartPtr(T *p = NULL) { ptr = p; }
    ~SmartPtr() { delete(ptr); }
    SmartPtr(const SmartPtr& other) = delete;
    SmartPtr(SmartPtr&& other) : ptr(other.ptr) { other.ptr = NULL; }
    SmartPtr& operator=(SmartPtr other)
    {
        std::swap(ptr, other.ptr);
        return *this;
    }

    T & operator * () const { return *ptr; }
    T * operator -> () const { return ptr; }

    bool operator<(const SmartPtr& other) const { return ptr < other.ptr; }
};

class simple {
    int x;
public:
    simple(int y = 0) : x(y) {}
    int getX() { return x; }
};
typedef SmartPtr<simple> simplePtr;

int main() {
    std::set<simplePtr> st;
    simplePtr p1 = simplePtr(new simple(5));
    simplePtr p2 = simplePtr(new simple(5));

    st.insert(std::move(p1));
    st.insert(std::move(p2));

    for (const simplePtr& p : st)
    {
        std::cout << p->getX();
    }
    return 0;
}

Answer 2

Your iterator needs to be dereferenced, at which point you get a pointer. Which then needs to be dereferenced. So:

    std::cout << (*it)->getX();

Answer 3

For starters you have to define operator < for the class SmartPtr before using it in the set.

Secondly you have to declare the member function getX like

int getX() const { return x; }

And you have to write at least like

std::cout << ( *it )->getX();