Jump to content
  • Advertisement

Archived

This topic is now archived and is closed to further replies.

xropi

Simple C++ question

This topic is 5728 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Look at the following example class:
    
class Cxxx {
protected:
  vector<int> *mpvec_x;           // or enything else

public:
  Cxxx() {};
  Cxxx(Cxxx &rSrc) {
    *this = rSrc;          // using our = operator for copy construction

  };
  Cxxx &operator=(Cxxx &rSrc) {
    mpvec_x = new vector<int>(*rSrc.mpvec_x);
    return *this;
  };
};
  
If all these are done by inline functions, is that efficient or should I implement the copy constructor the same way? Another question is that isn't it redundant? Isn't it enough if I define my = operation? (if I give one definition, the compiler may generate its own copy constructor using my = -- I think) [edited by - xropi on October 16, 2002 8:11:24 AM] [edited by - xropi on October 16, 2002 8:14:06 AM]

Share this post


Link to post
Share on other sites
Advertisement
Not any compiler I know about will generate a copy-constructor based on your overloaded operator= method. And you should be careful about doing it the way you are, if your operator= method deletes the mpvec_x before it reassigns it with a new one you could will into trouble if you haven''t set it to NULL before calling operator=.

Another thing: If you need to implement any of these three methods: A copy-constructor, an assignment operator (operator=) or a destructor you really should implement them all, otherwise you most probably get memory leaks.

Share this post


Link to post
Share on other sites
I think you should implement it in the copy-constructor, it will be more efficient. Especially if you add the following (simple) checks to make it more robust.

  
class Cxxx
{
public:
Cxxx()
: mpValue(new int(0))
{
}

Cxxx(const Cxxx& rOther)
: mpValue(new int(*rOther.mpValue))
{
}

~Cxxx()
{
delete mpValue;
}

Cxxx& operator=(const Cxxx& rOther)
{
if (this != &rOther)
{
delete mpValue;
if (rOther.mpValue == NULL)
{
mpValue = NULL;
}
else
{
mpValue = new int(*rOther.mpValue);
}
}

return this;
}

private:
int* mpValue;
};

Share this post


Link to post
Share on other sites
quote:
Original post by xropi
*this = rSrc; // using our = operator for copy

It is completely illegal to reassign "this". If you want to share implementation between cctor and assignment op, then have them both delegate to a private member function.

Share this post


Link to post
Share on other sites
quote:
Original post by SabreMan
[quote]Original post by xropi
*this = rSrc; // using our = operator for copy

It is completely illegal to reassign "this". If you want to share implementation between cctor and assignment op, then have them both delegate to a private member function.


I don''t change this at all! I change (*this) which is an instance of my Cxxx class and it''s operator = can be applied to that without pain.

(I tried it and (of course) it worked)

Share this post


Link to post
Share on other sites
Nevertheless, your code leaks and you should do like dalleboy told

But this

  
if (rOther.mpValue == NULL)
{
mpValue = NULL;
}

is useless if (mpValue != NULL) is an invariant to your class (like it is in dalleboy''s example).

Compiler''s copy constructor won''t use your operator=. It''ll use the default copy construction, which is a memberwise copy (and you don''t want that since you have pointer members)

In short, copy constructor should:
-copy the contents of other object

And assignment operator should:
-check for a=a assignments (and do nothing then)
-deallocate old memory
-copy the contents of other object

The last step is same on both so you could make a private function for that (and should, if there''s lots of stuff to copy, since otherwise it''s error-prone)

Share this post


Link to post
Share on other sites
Ok, it really had leak. Considering your point of view, civguy, I found this better:


    
class Cxxx {
protected:
vector<int> *mpvec_x; // or enything else

public:
Cxxx() {mpvec_x = NULL};
~Cxxx() {SAFE_DELETE(mpvec_x)};
Cxxx(Cxxx &rSrc) {
mpvec_x = new vector<int>(*rSrc.mpvec_x);
};
Cxxx &operator=(Cxxx &rSrc) {
if(this != &rSrc) {
~Cxxx();
Cxxx(rSrc);
}
return *this;
};
};



[edited by - xropi on October 16, 2002 12:57:53 PM]

Share this post


Link to post
Share on other sites
Heh, that can''t be legal, can it? Calling destructor and constructor explicitely from the member function, I mean. It may work on some compiler but I wouldn''t bet on it working on others..

Share this post


Link to post
Share on other sites
quote:
Original post by civguy
Heh, that can''t be legal, can it? Calling destructor and constructor explicitely from the member function, I mean. It may work on some compiler but I wouldn''t bet on it working on others..


I don''t see any reason not to call a constructor explicitly from a member function, because constructors/destructors don''t alloc/dealloc the storage for the object, the memory, where "this" points is an existing piece of memory.

The new and delete allocates and deallocates, but they act outside the object.

I will consult my C++ documentation shortly...

Share this post


Link to post
Share on other sites

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

Participate in the game development conversation and more when you create an account on GameDev.net!

Sign me up!