Smart but simple pointers

Question

For practice, I've implemented my own smart pointer class. It's a 'unique pointer', meaning it doesn't allow copies of itself (when copying from a into b, a is 'emptied' after the copying, i.e. the raw pointer is set to 0), so there can never be more than one time where delete is called on the raw pointer.

It utilizes RAII to bound the memory deallocation to the object lifetime, so it is just a basic smart pointer.

#ifndef SMARTPOINTER_CPP
#define SMARTPOINTER_CPP
#include <iostream>

template <typename T>
class SmartPointer {
public:
    SmartPointer(T* pointer = NULL) : pointer(pointer) { }
    ~SmartPointer(){
        delete pointer;
    }
    SmartPointer(SmartPointer& other){
        *this = other;
    }
    SmartPointer& operator=(SmartPointer& other){
        this->pointer = other.pointer;
        other.pointer = NULL;

        return *this;
    }
    T& operator*() const {
        return *pointer;
    }
    T* operator->() const {
        return pointer;
    }
private:
    T* pointer;
    template <typename E>
    friend std::ostream& operator<<
        (std::ostream& stream, const SmartPointer<E>& smartPointer);
};
template <typename E>
std::ostream& operator<<(std::ostream& stream, const SmartPointer<E>& smartPointer){
    stream << smartPointer.pointer;
}
#endif

Answer 1

This is not unique_ptr, it is auto_ptr, which has problems, mainly that copying a value modifies it, which is counter intuitive.

Assuming you really do not have move semantics and cannot do better (you should, C++11 is old now), there are still some problems:

• There is no way to release a pointer. If I am not happy with your SmartPointer I want to do something like SmartPointer<T> sp; shared_ptr<T> ssp = sp.release();.

• You forgot to return the stream inside ostream& operator <<. This should be a compilation error.

• There is no need for friend. I don't know who made it popular to make the ostream& operator << a friend but I'd like to give that person a stern look. Without friend this works just fine:

  template <typename E>
  std::ostream& operator<<(std::ostream& stream, const SmartPointer<E>& smartPointer){
      return stream << &(*smartPointer);
  }
  

• Your copy-constructor is wrong. It copies the pointer eventually resulting in a double delete. You probably meant to have other.pointer = nullptr;.

• Consider making the assignment operator return void. You are supposed to return *this to allow chaining like in a = b = c = nullptr;. However, since the assignment takes the value away there is no point chaining assignments, so better cause a compiler error.

• Your assignment operator causes a memory leak in the following case:

  SmartPointer<int> sp1 = new int{1};
  SmartPointer<int> sp2 = new int{2};
  sp1 = sp2;
  

  The memory for the first allocation is never deleted.
  
  Edit: There is a different memory leak case:

  SmartPointer<int> sp = new int;
  sp = sp;
  

I would suggest nullptr instead of NULL, but I guess your compiler does not know what that is.

Answer 2

Main problem is here:

SmartPointer& operator=(SmartPointer& other){

    this->pointer = other.pointer;  // you did not delete this->pointer
                                    // before overwritting it.
                                    // So you just leaked a bunch of memory.
    other.pointer = NULL;

    return *this;
}

This is why Assignment operators are usually written in terms of the copy constructor. In a patter called "Copy and Swap Idiom."

This is strange:

SmartPointer(SmartPointer& other){
    *this = other;
}

It just looks funky.
If I was going to write this I would have gone:

SmartPointer(SmartPointer& other){
    this->operator=(other);
}

But even that looks a bit strange as you normally define the assignment operator in terms of the copy constructor (see comment above).

Usage Problem:

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

Both of these are undefined behavior if the internal pointer is NULL (very possible). But there is no way to check the state of the internal pointer. So calling these methods is like playing Russian roulette.

Fix output operator

template <typename E>
std::ostream& operator<<(std::ostream& stream, const SmartPointer<E>& smartPointer){
    stream << smartPointer.pointer;
}

There does not seem to be a return in this function (yet it declares a return type).

Style

I see little point in declaring the output operator as a friend and then defining it two lines later. Do it all in a single spot inside the class:

template <typename T>
class SmartPointer {
public:
   // STUFF

    // Notice no need for template stuff here now.
    friend std::ostream& operator<<(std::ostream& stream, const SmartPointer& smartPointer){
         stream << smartPointer.pointer;
    }
};