1

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.

Improve this question
6
  • 2
    Hmm...I'm not sure what the purpose of this would be. The actual object itself already exhibits copy-semantics, so why even use a pointer? It's like you're reimplementing the semantics of an auto stack variable here. Commented Nov 6, 2010 at 12:51
  • 1
    That's what I want! As mentioned in the question, this improves memory locality. It's used in a simulation with some pretty heavy, written-for-clarity settings but where the settings aren't actually relevant while running the computation. I can't put the settings at the end of the object because there's inheritance going on and the settings are declared in an ancestor class. Commented Nov 6, 2010 at 12:57
  • 1
    You're not using a placement new or anything to control where the copy gets created, so why does having a copy of the object in a random place in the heap improve locality over having a copy of the object near the other local variables on the stack? What architecture is this on? Commented Nov 6, 2010 at 13:05
  • @Pete: Because the pointed-to object isn't used yet only takes up a T* among the heavily-used data. This T is apparently only used once or twice before or after the loop, but never in the loop. (And I'd expect Eamon to actually run a performance comparison after implementing this class.) Commented Nov 6, 2010 at 13:10
  • What Roger Pate said - once it works that is, right now, I'm getting heap corruption by using this class, possibly due to a bug elsewhere... And of course I'm oversimplifying slightly - the object is used (which is why it's included) but it's sorta like while(true){ loopUsingHeavyObjectOnly(); loopUsingEverythingButHeavyObject(); } Commented Nov 6, 2010 at 13:24

3 Answers 3

Reset to default
5

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

7 Comments

copy_ptr& operator=(copy_ptr rhs) { swap(*this, rhs); return *this; } friend void swap(copy_ptr &a, copy_ptr &b) { std::swap(a.item, b.item); }
Why not just define the assignment operator as copy_ptr& operator=(copy_ptr const& rhs) { *item = *rhs.item; return *this; }
Oh and thanks - this is indeed the bug! However, this wrapper doesn't fix my problems, though it's still a fine, small comprehensible bit of practice :-).
@Roger: right, this is indeed the classic copy and swap idiom :)
@Eamon: Once again this isn't safe since you cannot assume (because of the default constructor implementation) that this->item and rhs.item are both non null. Whenever you are developping RAII classes (which does not happen that often), it is easier to go the copy-and-swap road in order not to look, even in the presence of exception. This idiom also enforce Strong Exception Guarantee. The typical implementation was demonstrated by Roger Pate in his comment.
|
3

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.

Improve this answer

5 Comments

When using the above template, my app crashes with heap corruption in that destructor on app shutdown in .NET's GC finalizer thread, so in the process of finding the bug I added the harmless =0; just to test if I'm not somehow messing something up and e.g. copying the pointer and double-deleting somehow - but to no avail, and hence this question as to whether the class is sane in the first place :-).
@Eamon: You can only ever see that item has been nulled if you access a copy_ptr after it has been destroyed, and that would be UB. You may find the heap corruption in this dtor, but that's easily because the corruption happens elsewhere and you can't detect it until you use the heap (which delete does). — Though it looks like Mattieu's answer explains why.
@VJo: It's not const correctness, the original code is fine that way.
@Roger Pate What is it? The original will work fine, but your code is const correct. Const method returns const reference and const pointer. Non const, return non const reference
@VJo: It's semantics, compare vector<int>::iterator const to vector<int>::const_iterator.
1

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

Improve this answer

2 Comments

Thanks, I can certainly do something with that!
If you go the cloning road, make it compliant with Boost Clonability concept either by declaring a virtual Base* clone() const = 0; in your Base object or providing Base* new_clone(Base const*); free function. This will make your class usable with Boost Pointer Container library.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.