Is it a good practice to call the destructor explicitly and use placement new(this) in assignment op

[Michael Tsang]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Consider the following codes:

class Foo {
public:
// default constructor
Foo();
// l-value copy constructor
Foo(const Foo &);
// destructor
virtual ~Foo();
// assignment operator
Foo &operator=(const Foo &x) {
if(this != &x) {
this->~Foo();
new(this) Foo(x);
}
return *this;
}
private:
// private data omitted
};

Is the above Foo:perator=(const Foo &) written in good practice?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksjq5gACgkQG6NzcAXitM/sBwCeNar9lgasbrpSQ3U7Yln9Yu8U
uNUAn07Q/zHscyETayBwXYSt275noT0q
=zi3m
-----END PGP SIGNATURE-----

 

[Jonathan Lee]

Is the above Foo:perator=(const Foo &) written in good practice?

No, I really don't think so. Imagine if your copy constructor
throws. If the user recovers from the throw, your instance of
Foo will be invalid. I believe there was another recent thread
that pretty much decided calling the destructor led to undefined
behaviour anyway.

I believe the most common practice is to create a temp using
the copy constructor and then call a nothrow swap, i.e.,

Foo& operator=(const Foo& x) {
if (this != &x) Foo(x).swap(*this);
return *this;
}

That way, if the temp construction throws you don't damage
the value being assigned to, and the user has a hope of
recovering.

--Jonathan

 

[Michael Tsang]

No. Think about what happens when someone derives from this class and
writes a normal assignment operator.

Sorry I forgot to make my assignment operator virtual. Now consider the
following code:

class Foo {
public:
// default constructor
Foo();
// l-value copy constructor
Foo(const Foo &);
// destructor
virtual ~Foo();
// assignment operator
virtual Foo &operator=(const Foo &x) {
if(this != &x) {
this->~Foo();
new(this) Foo(x);
}
return *this;
}
private:
// private data omitted
};

class Bar : public Foo {
public:
// default constructor
Bar() : Foo(), a(), b() {
}
// l-value copy constructor
Bar(const Bar &x) : Foo(x), a(x.a), b(x.b) {
}
// destructor
~Bar() {
}
// assignment operator
Bar &operator=(const Bar &x) {
// assign base class subobject
Foo:perator=(x);
// assign member subobjects
a = x.a;
b = x.b;
return *this;
}
// other methods omitted
private:
int a;
int b;
};

I don't know what strange things will happen when Bar:perator=(const Bar
&) is called.

 

[James Kanze]

Update: Is this UB?

class Foo {
public:
// default constructor
Foo();
// l-value copy constructor
Foo(const Foo &);
// destructor
virtual ~Foo();
// assignment operator
virtual Foo &operator=(const Foo &x) {
if(this != &x) {
// disable virtual mechanism
this->Foo::~Foo();
new(this) Foo(x);
}
return *this;
}
private:
// private data omitted
};

class Bar : public Foo {
public:
// default constructor
Bar() : Foo(), a(), b() {
}
// l-value copy constructor
Bar(const Bar &x) : Foo(x), a(x.a), b(x.b) {
}
// destructor
~Bar() {
}
// assignment operator
Bar &operator=(const Bar &x) {
// assign base class subobject
Foo:perator=(x);
// assign member subobjects
a = x.a;
b = x.b;
return *this;
}
// other methods omitted
private:
int a;
int b;
};

Yes. In Bar:perator=, after calling Foo:perator=, there is
no more Bar object, and any use of the this pointer (including
simply returning it, I think) is undefined behavior.

Try it replacing the int's in Barr with tracer objects, objects
of a class type which logs all construction, destruction and
assignment, and you'll find yourself assigning to a destructed
object.

 

[Michael Tsang]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Pete Becker wrote:

Think in terms of implementation here: the constructor for a type sets
the vtable pointer to point to the vtable for that type. When you've
stomped on the vtable pointer with new(this)Foo(x) you do not have a Bar
object. You have a mongrel, with Bar's data and Foo's vtable pointer.

But technically, no, there's no undefined behavior here. There's no
behavior at all, since there's no code being executed. You'd have to
create and object, call operator=, and then do something with that object.

class Foo {
public:
// default constructor
Foo();
// l-value copy constructor
Foo(const Foo &);
// destructor
virtual ~Foo();
// assignment operator
virtual Foo &operator=(const Foo &x) {
if(this != &x) {
// disable virtual mechanism
this->Foo::~Foo();
new(this) Foo(x);
}
return *this;
}
private:
// private data omitted
};

class Bar : public Foo {
public:
// default constructor
Bar() : Foo(), a(), b() {
}
// l-value copy constructor
Bar(const Bar &x) : Foo(x), a(x.a), b(x.b) {
}
// destructor
~Bar() {
}
// assignment operator
Bar &operator=(const Bar &x) {
// assign base class subobject
Foo:perator=(x);
// assign member subobjects
a = x.a;
b = x.b;
return *this;
}
// other methods omitted
private:
int a;
int b;
};

int main() {
Bar b, c;
c = b; // UB?
}

So, is there any better method to write Foo:perator=(const Foo &) without
causing problem on Bar:perator=(const Bar &) if it is not changed?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksmR48ACgkQG6NzcAXitM+8iwCgh32S4LQu2Kz0TVp3BIDfhM99
fG4An1AXOmMSpajSTT0/9s9EFlHEQFPp
=8mDm
-----END PGP SIGNATURE-----

 

[Vladimir Jovic]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Pete Becker wrote:



class Foo {
public:
// default constructor
Foo();
// l-value copy constructor
Foo(const Foo &);
// destructor
virtual ~Foo();
// assignment operator
virtual Foo &operator=(const Foo &x) {
if(this != &x) {
// disable virtual mechanism
this->Foo::~Foo();
new(this) Foo(x);
}
return *this;
}
private:
// private data omitted
};

class Bar : public Foo {
public:
// default constructor
Bar() : Foo(), a(), b() {
}
// l-value copy constructor
Bar(const Bar &x) : Foo(x), a(x.a), b(x.b) {
}
// destructor
~Bar() {
}
// assignment operator
Bar &operator=(const Bar &x) {
// assign base class subobject
Foo:perator=(x);
// assign member subobjects
a = x.a;
b = x.b;
return *this;
}
// other methods omitted
private:
int a;
int b;
};

int main() {
Bar b, c;
c = b; // UB?
}

So, is there any better method to write Foo:perator=(const Foo &) without
causing problem on Bar:perator=(const Bar &) if it is not changed?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksmR48ACgkQG6NzcAXitM+8iwCgh32S4LQu2Kz0TVp3BIDfhM99
fG4An1AXOmMSpajSTT0/9s9EFlHEQFPp
=8mDm
-----END PGP SIGNATURE-----

1) Your PGP signature is very annoying

2) What kind of compiler you used to compile your example? You didn't
include <new>, missing definitions of Foo's constructor and destructor.

3) As an exercise, add this pure virtual method to Foo:
virtual void ops() = 0;
and implement it in Bar, like this:
virtual void ops(){}
Then call this method as the last step in the main(). When you compile
and run, you will get a nice surprise

 

[James Kanze]

It also doesn't make any further use of b.

As long as there is no further use of the this pointer, *nor* of
any other pointer to the object, there is no problem. (Note
that "delete this" is only appropriate for certain types of
objects, which explicitly manage their own lifetime, and will
take the appropriate steps in their destructor to ensure that no
other pointers to them remain. Of course, in a lot of
applications, no other types should be dynamically allocated to
begin with.)

The problem, of course, isn't that you're still going to use the
object. The problem is that you've changed its type.

In no way do I advocate actually using this, but here is an
example of using the object after assignment. UB or not UB?

#include <iostream>

class Foo {
public:
Foo(int x): x_(x) {}
void operator=(const Foo& b) {
void* p = static_cast<void*>(this);
this->~Foo();
new (p) Foo(b.x_);
}
virtual ~Foo() {}
virtual void f() {
std::cout << "Foo(" << x_ << ")\n";
}
protected:
int x_;
};

class Bar: public Foo {
public:
Bar(int x, int y): Foo(x), y_(y) {}
virtual void f() {
std::cout << "Bar(" << x_ << ", " << y_ << ")\n";
}
private:
int y_;
};

int main() {

Foo* p = new Bar(42,43);

std::cout << "Before assignment: ";
p->f();

*p = Foo(31);

std::cout << "After assignment: ";
p->f();

delete p;
}

Undefined behavior. The standard does allow constructing a new
object where an old one existed. But pointers to the old one
are only valid if the storage exactly overlays the old one (the
case here, maybe), the new object has the same type (modulo
cv-qualifiers) as the old one, and neither are base class
subobjects. If you want to see why, consider modifying Bar as
follows:

struct BarBase { int i; /* take some space */ };
class Bar : private BarBase, public Foo
{
// as before...
};

Now what do you get?

 

[James Kanze]

* Michael Tsang:

If the actual object was of a type derived from Foo, the above
has invalidated all of the pointers to it. I'm also not too
sure, but I think destructing the base class other than from the
destructor of the derived class is undefined behavior---it
should be, anyway.

Assignment operator should as a rule never be virtual, because
that moves error detection from compile time to run time, and
also, it's easy to forget to override it, as it seems you have
forgotten in the class below?

In other words, it creates a mess.

Not to forget that there's no way to implement the actual
semantics of assignment in a derived class. Consider:

struct Base { virtual ~Base() {}; }
class D1 : public Base { /*...*/ };
class D2 : public Base { /*...*/ };

Base p = new D1;
*p = D2() ;

What should the last line mean? How do you implement it.

If you need assignment in a polymorphic class, you can fake it
using something like the letter/envelop idiom, but the cost is
usually fairly high, and it's rarely appropriate.

The other problem is, as Pete noted above, that you're
stomping all over the vtable. It's just completely
unnecessary. What is the purpose that you /think/ you're
achieving (please forgive me for not reading up the whole
thread)?

Of course, given his implementation of Foo:perator=, there is
no more Bar object here, and any use of the this pointer after
this line is undefined behavior.

But as a general rule, the compiler generated assignment
operator is generally sufficient, and where it isn't, the fix
is most probably to equip member variables with proper copying
semantics instead of specializing the assignment operator and
other things to deal with their lack of proper copyability.

Well, you've got to start somewhere. But generally, in my
experience, a lot of classes shouldn't support assignment to
begin with. Which avoids a lot of the questioning as well.

For example, a raw pointer member variable might seem to
require some custom assignment operator in the containing
class. Solution: use a smart pointer. Not a solution:
implementing a custom assignment operator.

If the requirement is deep copy of some more or less complex
structure (e.g. like std::map), then you have to implement it.
The compiler will only do a shallow copy, and smart pointers
will only go so far.

For those few remaining cases where it's most practical to do
the custom assignment operator, you'll often find that that
the swap idiom for assignment serves your needs. This is a way
to express assignment in terms of copy construction, and it's
generally exception and self-copy safe. It goes like

T& operator=( T const& other )
{
T const copy( other ); // Using copy constructor to copy.
this->swap( copy ); // Can't throw, by design.
}

I wrote "this->" just to make it clear since it's a snippet
out of context; writing "this->" is not something that one
should do in real code...

More generally, you fully construct all necessary data, and do
anything else which might throw, before modifying the original
object. (The swap idiom is a convenient way of accomplishing
this in many cases, but for the very simplest cases, like a
pimpl using deep copy, it may be overkill, and be just as simple
to implement using the idiom which was standard before the swap
idiom was invented.)

 

[Alf P. Steinbach]

* James Kanze:

More generally, you fully construct all necessary data, and do
anything else which might throw, before modifying the original
object. (The swap idiom is a convenient way of accomplishing
this in many cases, but for the very simplest cases, like a
pimpl using deep copy, it may be overkill, and be just as simple
to implement using the idiom which was standard before the swap
idiom was invented.)

Hello ... Hello ... JAMES!

You forgot, there's a 'const' in there that shouldn't be there!

But OK, you probably thought it was just a typo and it was...


Cheers,

- Alf