CODEWITHSUNDEEP
c++c++11smart-pointers
Eamon Nerbonne
Eamon Nerbonne

Reputation: 48066

Is this C++ pointer container safe?

I want a safe C++ pointer container similar to boost's scoped_ptr, but with value-like copy semantics. I intend to use this for a very-rarely used element of very-heavily used class in the innermost loop of an application to gain better memory locality. In other words, I don't care about performance of this class so long as its "in-line" memory load is small.

I've started out with the following, but I'm not that adept at this; is the following safe? Am I reinventing the wheel and if so, where should I look?

template <typename T> 
class copy_ptr {
    T* item;
public:
    explicit copy_ptr() : item(0) {}
    explicit copy_ptr(T const& existingItem) : item(new T(existingItem)) {}
    copy_ptr(copy_ptr<T> const & other) : item(new T(*other.item)) {}
    ~copy_ptr()  { delete item;item=0;}

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

Edit: yes, it's intentional that this behaves pretty much exactly like a normal value. Profiling shows that the algorithm is otherwise fairly efficient but is sometimes hampered by cache misses. As such, I'm trying to reduce the size of the object by extracting large blobs that are currently included by value but aren't actually used in the innermost loops. I'd prefer to do that without semantic changes - a simple template wrapper would be ideal.

Upvotes: 1

Views: 475

Answers (3)

Matthieu M.
Matthieu M.

Reputation: 299760

No it is not.

You have forgotten the Assignment Operator.

You can choose to either forbid assignment (strange when copying is allowed) by declaring the Assignment Operator private (and not implementing it), or you can implement it thus:

copy_ptr& operator=(copy_ptr const& rhs)
{
  using std::swap;

  copy_ptr tmp(rhs);
  swap(this->item, tmp.item);
  return *this;
}

You have also forgotten in the copy constructor that other.item may be null (as a consequence of the default constructor), pick up your alternative:

// 1. Remove the default constructor

// 2. Implement the default constructor as
copy_ptr(): item(new T()) {}

// 3. Implement the copy constructor as
copy_ptr(copy_ptr const& rhs): item(other.item ? new T(*other.item) : 0) {}

For value-like behavior I would prefer 2, since a value cannot be null. If you go for allowing nullity, introduces assert(item); in both operator-> and operator* to ensure correctness (in debug mode) or throw an exception (whatever you prefer).

Finally the item = 0 in the destructor is useless: you cannot use the object once it's been destroyed anyway without invoking undefined behavior...

There's also Roger Pate's remark about const-ness propagation to be more "value-like" but it's more a matter of semantics than correctness.

Upvotes: 5

usta
usta

Reputation: 6869

In addition to what Roger has said, you can Google 'clone_ptr' for ideas/comparisons.

Upvotes: 1

Roger Pate
Roger Pate

Reputation:

You should "pass on" the const-ness of the copy_ptr type:

T* get() { return item; }
T& operator*() { return *item; }
T* operator->() { return item; }

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

Specifying T isn't needed in the copy ctor:

copy_ptr(copy_ptr const &other) : item (new T(*other)) {}

Why did you make the default ctor explicit? Nulling the pointer in the dtor only makes sense if you plan on UB somewhere...

But these are all minor issues, what you have there is pretty much it. And yes, I've seen this invented many times over, but people tend to tweak the semantics just slightly each time. You might look at boost::optional, as that's almost what you have written here as you present it, unless you're adding move semantics and other operations.

Upvotes: 3

Related Questions