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

• Loki Astari: It's a 'unique pointer' No its an auto ptr. You should implement move semantics to get functionality like unique ptr

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.

• vnp: A copy constructor is all right. It is implemented in terms of assignment, which does nullify other.pointer (verified with debugger).
• nwp: @vnp I agree with what you wrote, but it does not contradict what I wrote, which confuses me. The nullifying in the assignment works fine, but multiple assignments (like a = b = c;) do not make sense. It means the value moves from c to b to a. Why not move directly from c to a and skip b? Returning void from the assignment operator allows a = c; but not a = b = c;. It is a confusing way to nullify b.
• Loki Astari: I like declaring them as friends it makes sense to define tight coupling and not extend the interface. Also your way precludes the ability to test for NULL. It is obviously part of the interface to the class so friend is a good choice. programmers.stackexchange.com/a/99595/12917
• user52292: friend is needed if you immediatelly define the operator inside the class (not later outside). Here it can be done without it if you like &* more than accessing private pointer.. or even better: public get that should be there. Still, friending the operator is a matter of choice, not that bad.
• Aviv Cohn: What should release() do? Should it return the inner raw pointer and then null the member?

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;
 }
};

• user52292: Aren't you missing the friend on the operator<<? I thought it must be there if you define it inline (inside the class). See section2: Defines a non-member function, and makes it a friend of this class at the same time. Such non-member function is always inline.