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

Discussion in 'C++' started by Michael Tsang, Dec 12, 2009.

  1. -----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::eek: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-----
     
    Michael Tsang, Dec 12, 2009
    #1
    1. Advertisements

  2. Michael Tsang

    Jonathan Lee Guest

    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
     
    Jonathan Lee, Dec 12, 2009
    #2
    1. Advertisements

  3. 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::eek: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::eek:perator=(const Bar
    &) is called.
     
    Michael Tsang, Dec 13, 2009
    #3
  4. Michael Tsang

    James Kanze Guest

    Yes. In Bar::eek:perator=, after calling Foo::eek: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.
     
    James Kanze, Dec 14, 2009
    #4
  5. -----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::eek: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::eek:perator=(const Foo &) without
    causing problem on Bar::eek: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-----
     
    Michael Tsang, Dec 14, 2009
    #5
  6. 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 :)
     
    Vladimir Jovic, Dec 14, 2009
    #6
  7. Michael Tsang

    James Kanze Guest

    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.
    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, Dec 14, 2009
    #7
  8. Michael Tsang

    James Kanze Guest

    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.
    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.
    Of course, given his implementation of Foo::eek:perator=, there is
    no more Bar object here, and any use of the this pointer after
    this line is undefined behavior.
    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.
    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.
    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.)
     
    James Kanze, Dec 14, 2009
    #8
  9. * James Kanze:
    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
     
    Alf P. Steinbach, Dec 14, 2009
    #9
    1. Advertisements

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments (here). After that, you can post your question and our members will help you out.