Thread: Copying object that owns both variable and object owning pointer to variable

Oh boy, this is going to be hard for me to explain in detail using c++ I'm a noob in C++, be forewarned.

Originally Posted by Galdor

Yes, absolutely ! But how do you do that ? Having a custom copy constructor allows to do just that, but is it the only way ?

Lets talk about ownership. Ownership of a pointer means that the ptrOwner is responsible for managing the life time of the pointer myPtr, that includes its release from memory and in some cases the allocation as well. Only one object can be the owner, all others they only reference it, depending on the situation and code, it might be unwise to reference the pointer and instead reference the owner.

Any method, constructor, destructor etc of a class should never ever handle or care about externally managed data. When a copy is made the data managed internally by the class must always be copied, some external references usually require copying too but those are the references the old object already has and know, it should never care if there are other external objects with the same references as well. If I haven't misunderstood you, your wrapper class when copying it self should not care about the existence of ptrOwner at all nor it should try to update ptrOwner's references, simply put, it is wrong to do so, ptrOwner should take care if its own data.

As for the copy mechanism. I'm used to be able to copy the data between compatible classes any time I require to, not only during construction so all my classes have two methods called CopyFrom and CopyTo that can be invoked any time I need to do so. Although they are used mostly close to the construction of an object it is far more versatile this way.

Originally Posted by Galdor

You are suggesting me to use events :
I am not familiar with that concept, therefore I had a look on the internet and found this nice example. However, I don't really see the connection with my problem… How are events going to help me ? Perhaps you could give me a more precise example ?

That is what I called "a simple call back" in my previous post. This is the simplest most basic of event mechanisms and is the one I'll be using mostly in my libraries. Of course I do not use the constructor methodology that is shown in the article, the event must be able to change at any time, in the object's life time, not only during construction, so two methods are added for each event, one that sets the event's reference and one that reads the event's reference back, although the read part is never used so far, I fill more comfortable knowing that it is there, so fill free to skip it.

Originally Posted by Galdor

Also, by looking over the internet I realised that a lot of people seem to be using external libraries to handle events : here, there and here again. I assume that in my case you recommend me to implement those events myself, correct ? More generally, do you usually implement events yourself or do you prefer using an external library ?

I'm not familiar with those libraries I have a light understanding on signals/slots methodology from my recent studies with QT but I'm not comfortable commenting on their merits or problems, but yes a signal/slot is one way of handling events that falls in the single producer/multiple consumer methodology, the windows messaging system is an other, creating an array (link list, stuck etc) that holds a number of (*ichange) references is a third, I used all of those in my past and I have came to the realization that the events in class should be as simple as possible (aka the callback) and get extended as needed with external classes(ee an object that attaches it self to a classes event and all intersted parties attach them selfs to that). But as you see in the article it self an event put it simple is a way to request from an object to execute some user selected code with out the need to touch the class it self.

Assuming that my understanding of your problem is correct I would guess that this should answer your question.

But what if Wrapper owns an object which owns an object which owns a pointer to Wrapper::val_ ?

Well, note that it's important wrappers should achieve something. For example. it is useful to wrap a pointer in a class type to attain certain copy semantics and then use that. Wrapping code just for pretty-fication often has the opposite effect, and will only obfuscate and confuse readers.

Speaking of wrapped up, or "smart pointers", reference counting would solve your problem to a degree as well. The basic idea is you wrap a regular pointer and count how many times that pointer was copied. All of the copies will share the same memory. As the copies fall out of scope, the counter decreases, and when it is 0, the shared memory is deleted with the object that lived the longest. You might want to search the web about smart pointers, and look at an implementation of shared_ptr in particular to see what's going on with that technique.

I think I can implement a quick and dirty version in C++98:

Code:

#include <iostream>
#include <algorithm>


#define DISP_COUNT(ptr) \ 
do { \
    std::cout << "The count for " #ptr " is " << (ptr).count() << std::endl;	\
} while (0) 


class RefCountedPtr {
private:
    struct mem
    {
	int *ptr;
	unsigned count;
    } *ref_;


    void swap(RefCountedPtr& rhs) {
	std::swap(ref_, rhs.ref_);
    }


public:
    RefCountedPtr() : ref_(new mem) {
	ref_->ptr = 0; 
	ref_->count = 1;
    }
    RefCountedPtr(int *p) : ref_(new mem) {
	ref_->ptr = p; 
	ref_->count = 1;
    }
    RefCountedPtr(const RefCountedPtr& other) : ref_(other.ref_) {
	ref_->count++;
    }
    ~RefCountedPtr() {
	if (--ref_->count == 0) {
	    delete ref_->ptr;
	    delete ref_;
	}
    }
    RefCountedPtr& operator = (const RefCountedPtr& rhs) {
	RefCountedPtr temp(rhs);
	temp.swap(*this);
	return *this;
    }
    int& operator* () const {
	return *(ref_->ptr);
    }
    int& operator* () {
	return *(ref_->ptr);
    }
    int& operator-> () const {
	return *(ref_->ptr);
    }
    int& operator-> () {
	return *(ref_->ptr);
    }
    int count() const {
	return ref_->count;
    }
};


int main()
{
    RefCountedPtr p1(new int);
    *p1 = 4;
    RefCountedPtr p2(p1);
    std::cout << "In main() right now..." << std::endl;
    DISP_COUNT(p1);
    DISP_COUNT(p2);
    RefCountedPtr p3(new int);
    *p3 = 12;
    DISP_COUNT(p3);
    {
	std::cout << "Inside the new block..." << std::endl; 
	RefCountedPtr p4;
	p4 = p3;
	DISP_COUNT(p4);
	DISP_COUNT(p3);
    }
    std::cout << "Outside of the new block..." << std::endl;
    DISP_COUNT(p1);
    DISP_COUNT(p2);
    DISP_COUNT(p3);
    return 0;
}

For me it produces:
In main() right now...
The count for p1 is 2
The count for p2 is 2
The count for p3 is 1
Inside the new block...
The count for p4 is 2
The count for p3 is 2
Outside of the new block...
The count for p1 is 2
The count for p2 is 2
The count for p3 is 1

However it has been a long time since I needed to implement the reference counted pointer myself instead of using a first- or third-party version, so trust, but verify my design.

I would say that C++ programmers reach for something like this when ownership or object lifetime issues crop up, rather than use event-driven programming.

Isn't "a copy" imply that all of the "references" have a unique copy of the data in a way that when changed the other "references" should not see that change? If yes do you implement some kind of logic to accomplish that? Something along the lines of "copy on write" perhaps?

Well, I want to be sure I am properly understood here. I don't think C++ terminology comes with any of the restrictions you speak of. A reference is simply an alias for an object, full stop. What I was trying to communicate was that the new pointer, when it is copied in a "shallow" way -- basically there are two pointers now with the same value -- they will be counted. As these shallow copies go away, they are discounted.

If you want copy on write semantics, you can do that too, but I didn't attempt that here. The main reason is because memory is really shared - i.e. all changes are relevant to all copies of that thing.

Well reference counting is an all or nothing game ee if there is single reference with out raising the count it will be troublesome in which case an auto reference counting mechanism saves a lot of code and errors.

Yes, you can do it wrong - but that is what testing and debugging is for. Incorrect class design is an occupational hazard.

Originally Posted by whiteflags

Well, note that it's important wrappers should achieve something. For example. it is useful to wrap a pointer in a class type to attain certain copy semantics and then use that. Wrapping code just for pretty-fication often has the opposite effect, and will only obfuscate and confuse readers.

We agree on this ! This was just a dummy example to work from…

Originally Posted by whiteflags

Speaking of wrapped up, or "smart pointers", reference counting would solve your problem to a degree as well. The basic idea is you wrap a regular pointer and count how many times that pointer was copied. All of the copies will share the same memory.

I though smart pointers are for when you want to allocate data ? Here I just want to point to an already existing variable… And also, in the case the variable pointed to is an object : I do not want the smart pointer to call its destructor, that would be troublesome…

Eventually, in my case the pointers must not point to the same location in memory.

Originally Posted by taazz

Isn't "a copy" imply that all of the "references" have a unique copy of the data in a way that when changed the other "references" should not see that change?

I want each Wrapper object to have it's own "Wrapper::val_" variable. Then, each "Wrapper::PtrOwner::myPtr_" pointer must point to the corresponding "Wrapper::val_". No "Wrapper::val_" shall be "shared" by two different pointers.

However, if I use the default copy constructor/assignement operator, the value of the pointers are copied and I end up with two of the pointers pointing to the same place.

For instance, consider the following main function :

Code:

int main(void)
{
    Wrapper   WrpA;
    Wrapper   WrpB;

    std::cout << "Before copy"                                     << std::endl;
    std::cout << "\tAddress of variables pointed to :"             << std::endl;
    std::cout << "\t\tWrpA.val_ = " << &(WrpA.val_)                << std::endl;
    std::cout << "\t\tWrpB.val_ = " << &(WrpB.val_)                << std::endl;
    std::cout << "\tAddresses stored in owned pointers :"          << std::endl;
    std::cout << "\t\tWrpA.MyOwn_.myPtr_ = " << WrpA.MyOwn_.myPtr_ << std::endl;
    std::cout << "\t\tWrpB.MyOwn_.myPtr_ = " << WrpB.MyOwn_.myPtr_ << std::endl;
    std::cout << std::endl;

    WrpB = WrpA;

    std::cout << "After copy"                                      << std::endl;
    std::cout << "\tAddress of variables pointed to :"             << std::endl;
    std::cout << "\t\tWrpA.val_ = " << &(WrpA.val_)                << std::endl;
    std::cout << "\t\tWrpB.val_ = " << &(WrpB.val_)                << std::endl;
    std::cout << "\tAddresses stored in owned pointers :"          << std::endl;
    std::cout << "\t\tWrpA.MyOwn_.myPtr_ = " << WrpA.MyOwn_.myPtr_ << std::endl;
    std::cout << "\t\tWrpB.MyOwn_.myPtr_ = " << WrpB.MyOwn_.myPtr_ << std::endl;
    std::cout << std::endl;

    return 0;
}

Here "WrpB.MyOwn_.myPtr_" must always be different from "WrpA.MyOwn_.myPtr_" .
This will be the case if the following copy constructor and assignment operators are defined

Code:

    Wrapper::Wrapper(const Wrapper & other) : val_ (other.val_), MyOwn_(this->val_) {}
    Wrapper & operator= (const Wrapper & other) { val_ = other.val_; MyOwn_.myPtr_ = &val_; return *this; }

Code:

Before copy
    Address of variables pointed to :
        WrpA.val_ = 0x7fff32ce9638
        WrpB.val_ = 0x7fff32ce9628
    Addresses stored in owned pointers :
        WrpA.MyOwn_.myPtr_ = 0x7fff32ce9638
        WrpB.MyOwn_.myPtr_ = 0x7fff32ce9628

After copy
    Address of variables pointed to :
        WrpA.val_ = 0x7fff32ce9638
        WrpB.val_ = 0x7fff32ce9628
    Addresses stored in owned pointers :
        WrpA.MyOwn_.myPtr_ = 0x7fff32ce9638
        WrpB.MyOwn_.myPtr_ = 0x7fff32ce9628

With the default copy constructor and assignment operator it won't…

Code:

Before copy
    Address of variables pointed to :
        WrpA.val_ = 0x7fff32ce9638
        WrpB.val_ = 0x7fff32ce9628
    Addresses stored in owned pointers :
        WrpA.MyOwn_.myPtr_ = 0x7fff32ce9638
        WrpB.MyOwn_.myPtr_ = 0x7fff32ce9628

After copy
    Address of variables pointed to :
        WrpA.val_ = 0x7fff32ce9638
        WrpB.val_ = 0x7fff32ce9628
    Addresses stored in owned pointers :
        WrpA.MyOwn_.myPtr_ = 0x7fff32ce9638
        WrpB.MyOwn_.myPtr_ = 0x7fff32ce9638

In the first example I posted (see original post), both pointers point to "TmpWrp.val_", then TmpWrp is destroyed ("delete TmpWrp") so I end up with "MyWrp.MyOwn_.myPtr_" pointing in the wild !

Originally Posted by Galdor

Actually I expect(ed) it to seg fault if my class is not properly implemented. This is a toy problem, it is not part of a testing framework, so do we really care ?

That's not what I said. In the code running the test, you were referring to a deleted object. That might result in a seg-fault that has nothing to do with the class being broken per se, so I would hope you care. With a buggy test, all you end up doing is looking for a problem in your class that isn't there.

I thought that the topic had been covered but after giving it another look today it seems it's not…

This :

Originally Posted by whiteflags

Code:

    std::swap(this->val_, other.val_);
    std::swap(this->MyOwn_, other.MyOwn_);

is not enough to give the right values to "Wrapper::PtrOwner::myPtr_" . As a solution, I added calls to a setter method.

Code:

        std::swap( this->val_  , other.val_   );
        std::swap( this->MyOwn_, other.MyOwn_ );

        this->MyOwn_.setPtr( &(this->val_) );
        other.MyOwn_.setPtr( &(other.val_) );

It works but was that the right way to achieve this ?

---

The full code :

Code:

#include <cassert>
#include <iostream>
#include <vector>

///////////////////////////////////////////////////////////

class PtrOwner
{
public:
    //Constructor
    PtrOwner(double & var) : myPtr_ (&var) {}
    //Default copy/move constructor/assign-op are used

    //Getters & setters
    double * getPtr(void) { return myPtr_; }
    void     setPtr(double * newPtr) { myPtr_ = newPtr; }

    //Attributes (would be private in an actual code)
    std::vector<double> massiveVar; //Member variable that's too "massive" to be copied cheaply
    double * myPtr_;
};

///////////////////////////////////////////////////////////

class Wrapper
{
public:

    //Constructor
    Wrapper(void) : val_ (2.0), MyOwn_ (val_) {}

    //Copy constructor
    Wrapper(const Wrapper &  other)
        :val_  (other.val_  )
        ,MyOwn_(other.MyOwn_)
    {
        this->MyOwn_.setPtr( &(this->val_) ); 
    }

    //Move constructor
    Wrapper(/***/ Wrapper && other)
        :val_  ( std::move(other.val_  ) )
        ,MyOwn_( std::move(other.MyOwn_) )
    {
        this->MyOwn_.setPtr( &(this->val_) ); 
    }

    //Copy/move assignment operator
    Wrapper & operator= (Wrapper other)
    {
        other.swap(*this);
        return *this;
    }

    //Swap function
    void swap(Wrapper& other)
    {
        std::swap( this->val_  , other.val_   );
        std::swap( this->MyOwn_, other.MyOwn_ );

        this->MyOwn_.setPtr( &(this->val_) );
        other.MyOwn_.setPtr( &(other.val_) );
    }

    //Attributes (would be private in an actual code)
    double    val_;
    PtrOwner  MyOwn_;
};

///////////////////////////////////////////////////////////

bool
test_objIsConsistent
        (const Wrapper & myWrp
        )
{
    return myWrp.MyOwn_.myPtr_ == &(myWrp.val_);
}

///////////////////////////////////////////////////////////

void dispInfo
        (const std::string objNm //Name of object
        ,Wrapper & Wrp           //Object
        )
{
    std::cout << "\t";
    std::cout << objNm + ".val_ = "          << static_cast<void*>(&(Wrp.val_))       << " ; ";
    std::cout << objNm + ".MyOwn_.myPtr_ = " << static_cast<void*>(Wrp.MyOwn_.myPtr_) << " ; ";
    std::cout << std::endl;
}

///////////////////////////////////////////////////////////

int main(void)
{
    //CONSTRUCTOR

    Wrapper   WrpA;

    std::cout << "After construction" << std::endl;
    dispInfo( "WrpA", WrpA );
    std::cout << std::endl;

    assert( test_objIsConsistent(WrpA) );

    //COPY CONSTRUCTOR

    Wrapper   WrpB = WrpA;

    std::cout << "After copy construction" << std::endl;
    dispInfo( "WrpA", WrpA );
    dispInfo( "WrpB", WrpB );
    std::cout << std::endl;

    assert( test_objIsConsistent(WrpA) );
    assert( test_objIsConsistent(WrpB) );

    //MOVE CONSTRUCTOR

    Wrapper   WrpC = std::move(WrpA);

    std::cout << "After move construction" << std::endl;
    dispInfo( "WrpA", WrpA );
    dispInfo( "WrpC", WrpC );
    std::cout << std::endl;

    assert( test_objIsConsistent(WrpA) );
    assert( test_objIsConsistent(WrpC) );

    //COPY ASSIGNMENT OPERATOR

    Wrapper  WrpD;
    WrpD = WrpA;

    std::cout << "After copy assignment" << std::endl;
    dispInfo( "WrpA", WrpA );
    dispInfo( "WrpD", WrpD );
    std::cout << std::endl;

    assert( test_objIsConsistent(WrpA) );
    assert( test_objIsConsistent(WrpD) );

    //MOVE ASSIGNMENT OPERATOR

    Wrapper  WrpE;
    WrpE = std::move(WrpA);

    std::cout << "After move assignment" << std::endl;
    dispInfo( "WrpA", WrpA );
    dispInfo( "WrpE", WrpE );
    std::cout << std::endl;

    assert( test_objIsConsistent(WrpA) );
    assert( test_objIsConsistent(WrpE) );

    return 0;
}

I made a mistake. You have to swap PtrOwner::MyPtr_ to make the swap correct. Getters and setters are not strictly necessary. As I had it written, I swapped the entire PtrOwner object, expecting PtrOwner to do the right thing. If you do nothing else, that should be the fix you are looking for. Note as well that swapping the entire PtrOwner object results in temporary copies of the massive vector.

I have to admit I am getting more and more nervous about the design. If you are reaching into the internals of massive vector, I would scrap all of this.

Code:

class HeavyObject
{
private:
   vector<double> _massiveVar;

public:

   typedef Iterator vector<double>::iterator;
//...
   Iterator Begin() 
   {
      return massiveVar.begin();
   }

   Iterator IteratorAt( size_t idx ) 
   {
      return massiveVar.begin() + idx;
   }

   Iterator End() 
   {
      return massiveVar.end();
   }
};

You can give any function that is meant to modify the massive vector cheap-to-copy, expendable HeavyObject::Iterators instead.