Violating abstractions

I saw a piece of code on one of Andrew Koenig's articles (it is not his code by the way! he criticizes it and rightly so). It was an implementation of the assignment operator. Here it goes:

Code:

---

class A {
  public:
      A& operator=(const A& t) {
        this->~A();
        new(this) A(t);
        return *this;
      }
      // other members
};

---

I just wanted to post it here and know what people wanted to say about it. Go for it! :)

:eek: If one of my devlopers ever wrote this type of code [without pages of documentation justifing it, that was approved by the review commitee), he/she would no longer be developing software for a living.

Main problem : Exception safety.
If the copy constructor throws an exception, behavior is undefined.
Thus, in that case, this class doesn't even met the basic exception guarantee: namely, consistency.

i always call operator= in the constructor.

In terms of efficiency, it doesn't recycle memory if internally uses dynamic memor, of course, if the concrete implementation uses it, and must construct/destruct all internal objects instead of reset their states. This may imply an important overhead (depending on concrete implementation).

In terms of clarity i think it's better to implemtent an initialize/destroy pair.

yes you are right for classes that are based on aray/vectors will lose alot of speed.

This is also discussed by Sutter in one of his GoTW items, cant remember which one.

Quote:

---

Originally Posted by exterminator

I saw a piece of code on one of Andrew Koenig's articles (it is not his code by the way! he criticizes it and rightly so). It was an implementation of the assignment operator. Here it goes:

Code:

---

class A {
  public:
      A& operator=(const A& t) {
        this->~A();
        new(this) A(t);
        return *this;
      }
      // other members
};

---

I just wanted to post it here and know what people wanted to say about it. Go for it! :)

---

Without looking at any of the replies ...

What happens if A has a virtual destructor, and/or the operator= was called from a derived class' operator= function ?

Quote:

---

Originally Posted by SuperKoko

If the copy constructor throws an exception, behavior is undefined.

---

Yes, that too.
:D

Quote:

---

Originally Posted by Zaccheus

Without looking at any of the replies ...

What happens if A has a virtual destructor, and/or the operator= was called from a derived class' operator= function ?

---

The derived class object is destroyed, and a base class object is created at this place... Then, accessing/invoking any member of the derived class becomes UB.

Indeed. :)

Good responses! You have found the most complex ones (exception safety and the difference in static and dynamic type of the object of derived upon assignment if this one is its base)! :thumb:

But the easiest one that you missed was - check for self assignment.. :)

I think, virtual destructor alone will not cause problems, this class needs to be derived for the problem to show up, isn't it? Anyways, virtual destructor is enough hint about possibility of future extensions.

Quote:

---

Originally Posted by TheCPUWizard

:eek: If one of my devlopers ever wrote this type of code [without pages of documentation justifing it, that was approved by the review commitee), he/she would no longer be developing software for a living.

---

:D

Quote:

---

Originally Posted by Carlos Martinez

In terms of efficiency, it doesn't recycle memory if internally uses dynamic memor, of course, if the concrete implementation uses it, and must construct/destruct all internal objects instead of reset their states. This may imply an important overhead (depending on concrete implementation).

In terms of clarity i think it's better to implemtent an initialize/destroy pair.

---

I am not sure, but are any of your points in support of that code? :)

Why is the virtual destructor called at all? An assignment operator is supposed to assign values, not destroy them.

Flaws aside, what was the author of that snippet trying to accomplish exactly?

Quote:

---

Originally Posted by exterminator

check for self assignment.. :)

---

and what makes u think that is neccesary.

Quote:

---

Originally Posted by Mitsukai

and what makes u think that is neccesary.

---

Because it's assigning to itself. The first thing it does is to destroy itself, then it tries to recreate itself based on... the object that was just destroyed.

Quote:

---

Because it's assigning to itself. The first thing it does is to destroy itself, then it tries to recreate itself based on... the object that was just destroyed.

---

Which is really BAD, and definately Undefined Behavior.....but....

In many cases will appear to work :eek: :thumbd:

no it will never work..

because it wont be itselve anymore becus its destroyed so u assigned a value to the class but isntead of resulting in that value it results in being a destroyed object

If the constructor does not initialize any variables, and there are no other threads and the system does not pre-initialize memory, then the memory from the old instance will not have been written to.

Again BAD, and UNDEFINED

Quote:

---

Originally Posted by exterminator

But the easiest one that you missed was - check for self assignment.. :)

---

Yes, I did miss that, but after seeing the object commit suicide ... self assignment was fairly irrelevant anyway.
:D

Quote:

---

Originally Posted by Zaccheus

Yes, I did miss that, but after seeing the object commit suicide ... self assignment was fairly irrelevant anyway.

---

Objects do commit suicide sometimes and that can make sense.. but of course not in this case. I don't think it is irrelevant because usually ignoring self assignment is no trouble at all. All that it costs you is a few extra statements/instructions that can be done without. It saves you the time wasted upon various assignments that can be skipped using a simple check for the objects. That can be done here too but for that I would need to change the magnum opus.

Quote:

---

I don't think it is irrelevant because usually ignoring self assignment is no trouble at all. All that it costs you is a few extra statements/instructions that can be done without.

---

[code]
class SubEntity
{
SubEntity(SubEntity const &src)
{
Sleep(1000); // Expensive operations
}
};

class Entity
{
SubEntity m_Member;

Entity & operator = (Entity const &rhs)
{
// NOT Checking for Self has a magor performance impact!
}
}
[code]

Plus even if you assignment operator is "safe" for self assigns (without the check today), there is no guarentee that something will not change in the class six months down the road, and break your code.

ALWAYS check for self assigment and "short-circuit".


}

Quote:

---

Originally Posted by exterminator

Objects do commit suicide sometimes and that can make sense.. but of course not in this case.

---

Indeed.

Quote:

---

Originally Posted by exterminator

I don't think it is irrelevant because usually ignoring self assignment is no trouble at all. All that it costs you is a few extra statements/instructions that can be done without. It saves you the time wasted upon various assignments that can be skipped using a simple check for the objects. That can be done here too but for that I would need to change the magnum opus.

---

All I meant was that having seen how dangerous the code is, I did not look at it any further.
;)

I WAS NOT SAYING YOU DON'T HAVE TO CHECK FOR SELF ASSIGNMENT. ALWAYS CHECK FOR SELF ASSIGNMENT.
:wave: