Archived

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

Simple C++ question

This topic is 5538 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
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
quote:
Original post by civguy
Calling destructor and constructor explicitely from the member function, I mean.

The dtor call is illegal and the ctor call isn''t a ctor call. I''ll let you figure out what it really is.

Share this post


Link to post
Share on other sites
quote:
Original post by xropi
I don''t see any reason not to call a constructor explicitly from a member function

How about the fact that it''s not possible?

Share this post


Link to post
Share on other sites
quote:
Original post by SabreMan
[quote]Original post by xropi
I don''t see any reason not to call a constructor explicitly from a member function

How about the fact that it''s not possible?

Ok, if this is a fact, I''ll give it up. :-)

Share this post


Link to post
Share on other sites
Guest Anonymous Poster
silly question then where do you put the ctor and dtor if not as member functions?
or is that only for his example?

-Alpha_ProgDes (sorry to lazy to login in)

Share this post


Link to post
Share on other sites
With VC++ 6.0 (with SP5) the following sample works fine anyway:
(it seems be possible to call ctors dtors explicitly from member functions )


  

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

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

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

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

int main(int argc, char* argv[]) {
int i;

Cxxxy *xc, xb; // either Cxxx or Cxxxy will work

i = 0;
xc = new Cxxxy;
xc->mpvec_x = new vector<int>;
xc->mpvec_x->push_back(i++);
xc->mpvec_x->push_back(i++);
xc->mpvec_x->push_back(i++);
xb = *xc;

Cxxxy yy(xb);
for(i = 0; i < 3; i++) {
cout << (*yy.mpvec_x)[i] << " ";
}
delete xc;

return 0;
}

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..


It''s legal to call the destructor.
There is a common example which should NOT be used:

NOTE: this example is really BAD.

  
Cxxx& Cxxx::operator=(const Cxxx& other)
{
if (this != &other)
{
this->~Cxxx();
new (this) Cxxx(other);
}
return *this;
}


Of course the copy constructor can''t depend on the operator= .


So you see, this will compile and is legal.
And don''t even try to use this code with derived classes. (slicing)



schiggl

Share this post


Link to post
Share on other sites

        class Cxxx {
protected:
vector<int> mvec_x; // Not a pointer

public:
Cxxx() {}; // No need for an explicit copy constructor

};

Shigl: Legal but ... yuck. As you say, it's horribly dangerous (custom allocators also come to mind...)

xropi: how about that bit, still without a pointer -

  Cxxx& Cxxx::operator=(Cxxx other) // NOT const, NOT by reference

{
mvec_x.swap( other.mvec_x );
return *this;
}

Implementing assignment in term of the copy constructor.
Will be expensive in case of self-assigment (goes through unneeded copy).

Documents [ GDNet | MSDN | STL | OpenGL | Formats | RTFM | Asking Smart Questions ]
C++ Stuff [ MinGW | Loki | SDL | Boost. | STLport | FLTK | ACCU Recommended Books ]


[edited by - Fruny on October 16, 2002 1:50:50 PM]

Share this post


Link to post
Share on other sites
quote:
Original post by SabreMan
The dtor call is illegal and the ctor call isn''t a ctor call. I''ll let you figure out what it really is.
Heh, true.. Now that I gave it a second thought, it just creates a temp object

Share this post


Link to post
Share on other sites
quote:
Implementing assignment in term of the copy constructor.
Will be expensive in case of self-assigment (goes through unneeded copy).

Since vector::swap needs a non-const argument, I would have thought you''d want to remove the ''const''.

Why not:

  
class Cxxx
{
private:
auto_ptr<vector<int> > vec;
// other members here

public:
Cxxx() : vec(new vector<int>)
{}
Cxxx(const Cxxx& rhs) : vec(new vector<int>(*(rhs.vec))) // etc.

{}
void swap(const Cxxx& rhs) throws()
{
swap(vec, rhs.vec);
// and any other members

}
Cxxx& operator=(const Cxxx& rhs)
{
if(this == &rhs) return *this;
Cxxx temp(rhs);
temp.swap(*this);
return *this;
}
};

It doesn''t copy unnecessarily, and won''t fuck up the left-hand object unnecessarily.

Share this post


Link to post
Share on other sites
may I ask why in the hell you are dynamically allocating that vector? It seems completely redundant and useless.

You could achieve the same effect like this:


  
class Cxxx
{
protected:
std::vector<int> m_vecx;
};




The default copy ctor and assignment operators are fine. They''ll call the appropriate ctors and operators for the vector, which will copy everything over from vector to vector.

Share this post


Link to post
Share on other sites
quote:

may I ask why in the hell you are dynamically allocating that vector? It seems completely redundant and useless.

You could achieve the same effect like this:

The default copy ctor and assignment operators are fine. They''ll call the appropriate ctors and operators for the vector, which will copy everything over from vector to vector.




Of course, but if you read my comment carefully, you will see that it might have been anything else. (a pointer to a base class and then while instantiating, assign a derived object''s address for exmpl.)

The intent of the example was a "real" situation, in which the default compiler generated code doesn''t work correctly so we need to write them by hand.


Others: Thank you for your patience. I will implement my things using adjunct member functions like MakeEmpty() and Assign().


I don''t understand all that you wrote. You told me "you musn''t do that" but I still cannot figure out the reason. (it''s not a real reason that "because the C++ standard doesn''t allow this"; a real reason is the existence of a situation in which we were in trouble and that''s why the C++ standard excludes it)

I read in the MSDN the related topics but there was nothing about callinng destructors from member functions but I belive you. Although I, personally, don''t see any exact case when this would be a problem.

Share this post


Link to post
Share on other sites