The "stealing code from the destructor" bug

[gw7rib]

I've been bitten twice now by the same bug, and so I thought I would
draw it to people's attention to try to save others the problems I've
had. The bug arises when you copy code from a destructor to use
elsewhere.

For example, suppose you have a class Note. This class stores some
text, as a linked list of lines of text. The destructor runs as
follows:

Note::~Note() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

which works fine. You now decide it would be nice to be able to clear
the text of a note, using this code, so you split it up as follows:

Note::~Note() {
cleartext();
}

void Note::cleartext() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

So now you have an extra function you can use. And the destructor is
calling a neat, modular function, which is arguably better style than
the destructor being a sprawl of code.

The snag is that your program then mysteriously goes wrong. Worse
still, because the above is only a minor change to code that was
working perfectly, you tend to assume that the problem must instead lie
in the other changes you made at the same time - the code that uses
cleartext.

For those who haven't spotted the snag, the problem is this. The
destructor didn't try to leave the Note in a usable state, because the
Note was not going to be used again. Cleartext does however need to
leave the Note in a usable state, so it needs a line:

firstline = NULL;

to be added at the end. In fact, it also needs lastline = NULL; and a
few other additions.

Is this a bug that has been described before? If not, do I get to name
it? I was originally going to call it the "nicking code from the
destructor" bug but seeing as these newsgroups have an international
audience I think I will go for the "stealing code from the destructor"
bug. I've only hit the bug in C++ but presumably similar problems can
arise in any language that has some equivalent to a destructor.

 

[Alf P. Steinbach]

* (e-mail address removed):

I've been bitten twice now by the same bug, and so I thought I would
draw it to people's attention to try to save others the problems I've
had. The bug arises when you copy code from a destructor to use
elsewhere.

For example, suppose you have a class Note. This class stores some
text, as a linked list of lines of text. The destructor runs as
follows:

Note::~Note() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

Although this is clearly an example construed to illustrate the bug, I
think it's prudent to mention, for readers of this group, that the
standard provides a class std::list (which however currently requires
elements to be assignable, but that "bug" in the standard has been
corrected for the next version of the standard).

Using a std::list the Note destructor could be empty, because the
std::list destructor would do the work.

And with an empty destructor, no problem with reusing that
(non-existent) code!

which works fine. You now decide it would be nice to be able to clear
the text of a note, using this code, so you split it up as follows:

Note::~Note() {
cleartext();
}

void Note::cleartext() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

So now you have an extra function you can use. And the destructor is
calling a neat, modular function, which is arguably better style than
the destructor being a sprawl of code.

The snag is that your program then mysteriously goes wrong. Worse
still, because the above is only a minor change to code that was
working perfectly, you tend to assume that the problem must instead lie
in the other changes you made at the same time - the code that uses
cleartext.

For those who haven't spotted the snag, the problem is this. The
destructor didn't try to leave the Note in a usable state, because the
Note was not going to be used again. Cleartext does however need to
leave the Note in a usable state, so it needs a line:

firstline = NULL;

to be added at the end. In fact, it also needs lastline = NULL; and a
few other additions.

If destructor code is to be "reused", then most likely the class has a
default "empty" state established by default construction.

In that case one nice technique is to implement a swap() function, which
should never throw but just swap the contents of two objects, and then write

void Note::clear()
{
Note().swap( *this );
}

Voilà, the destructor code has been reused, completely safely!

Plus, an assignment operator can now reuse the copy constructor (if the
class has a copy constructor):

Note& Note:perator=( Note const& other )
{
Note( other ).swap( *this );
return *this;
}

Plus, that swap operation might come in very handy for other things.

Is this a bug that has been described before? If not, do I get to name
it? I was originally going to call it the "nicking code from the
destructor" bug but seeing as these newsgroups have an international
audience I think I will go for the "stealing code from the destructor"
bug. I've only hit the bug in C++ but presumably similar problems can
arise in any language that has some equivalent to a destructor.

I don't know, but I haven't heard about it, so perhaps it's a new one!

 

[Doug]

I've been bitten twice now by the same bug, and so I thought I would
draw it to people's attention to try to save others the problems I've
had. The bug arises when you copy code from a destructor to use
elsewhere.

For example, suppose you have a class Note. This class stores some
text, as a linked list of lines of text. The destructor runs as
follows:

Note::~Note() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

which works fine. You now decide it would be nice to be able to clear
the text of a note, using this code, so you split it up as follows:

Note::~Note() {
cleartext();
}

void Note::cleartext() {
Line *lin, *lin2;

lin = firstline;
while (lin) {
lin2 = lin;
lin = lin -> next;
delete lin2; }
}

So now you have an extra function you can use. And the destructor is
calling a neat, modular function, which is arguably better style than
the destructor being a sprawl of code.

The snag is that your program then mysteriously goes wrong. Worse
still, because the above is only a minor change to code that was
working perfectly, you tend to assume that the problem must instead lie
in the other changes you made at the same time - the code that uses
cleartext.

For those who haven't spotted the snag, the problem is this. The
destructor didn't try to leave the Note in a usable state, because the
Note was not going to be used again. Cleartext does however need to
leave the Note in a usable state, so it needs a line:

firstline = NULL;

to be added at the end. In fact, it also needs lastline = NULL; and a
few other additions.

Is this a bug that has been described before? If not, do I get to name
it? I was originally going to call it the "nicking code from the
destructor" bug but seeing as these newsgroups have an international
audience I think I will go for the "stealing code from the destructor"
bug. I've only hit the bug in C++ but presumably similar problems can
arise in any language that has some equivalent to a destructor.

Hiya there,

I'd just call this a simple 'violation of invariant' bug. The
invariant here is that at all times where your Note can be accessed it
is in a consistent and correct state. Your initial cleartext()
function left the object in an incorrect state, as you point out, thus
breaking the invariant. (Your destructor obviously does not need to
obey this, as the object is subsequently inaccessible [by a correct
program].)

(You can programmatically check invariants. For example, for a really
complicated object, you could create a function that checks the
invariants and asserts if there's a problem. You could then call this
function in debug code on entry to and exit from each function (e.g.
build it into your debug tracing). This will quickly let you know if
you **** up. There are all sorts of names I've heard for this idea,
but the only one that springs to mind right now is 'object
validation'.)

I personally call this bug the 'doh, not again' bug, but since you've
come up with real example of how you can inadvertantly do it, yeah, I
think you should feel free to call it whatever you want

Doug

 

[Arthur J. O'Dwyer]

* (e-mail address removed):

(FWIW, I don't think this bug needs a name beyond the existing
terminology "stupid mistake." Or "cut-and-paste without double-
checking to make sure the code is appropriate.")

If destructor code is to be "reused", then most likely the class has a
default "empty" state established by default construction.

In that case one nice technique is to implement a swap() function, which
should never throw but just swap the contents of two objects, and then write

void Note::clear()
{
Note().swap( *this );
}

Voila, the destructor code has been reused, completely safely!

Plus, an assignment operator can now reuse the copy constructor
(if the class has a copy constructor):

Note& Note:perator=( Note const& other )
{
Note( other ).swap( *this );
return *this;
}

Clever. It took me a few minutes to figure out what was going on; I
don't often see the result of a constructor being used as an object,
except in the special case of std::string("foo"). So anyone who uses
this technique in code I'm going to see had better put in a comment
or three! But once all your maintenance programmers learn how to
read this idiom, it does seem like it would save some time and LOC.

-Arthur

 

[Alf P. Steinbach]

* Arthur J. O'Dwyer:

Clever. It took me a few minutes to figure out what was going on; I
don't often see the result of a constructor being used as an object,
except in the special case of std::string("foo"). So anyone who uses
this technique in code I'm going to see had better put in a comment
or three! But once all your maintenance programmers learn how to
read this idiom, it does seem like it would save some time and LOC.

The main reason to use this idiom, apart from simplicity, is exception
safety (and yes, it's clever, but unfortunately not my invention).

Cheers,

- Alf

 

[David]

Hello,

I've been bitten twice now by the same bug, and so I thought I would
draw it to people's attention to try to save others the problems I've
had. The bug arises when you copy code from a destructor to use
elsewhere.

Any time you copy or refactor code it will be reused in slightly
different ways than it was originally. You may even take
apparently working code and substitute new values and have probems.

Details matter and it is important to check them all. Yes, we
all make such a mistake at one time or another. It isn't limited
to reusing a destructor.

For those who haven't spotted the snag, the problem is this. The
destructor didn't try to leave the Note in a usable state, because the
Note was not going to be used again. Cleartext does however need to
leave the Note in a usable state, so it needs a line:

This is one reason to always have your code leave everything in a
stable state. At some point you may reuse code or add something
before or after working code that changes its behavior. It is
a good idea to specify everything.

Is this a bug that has been described before? If not, do I get to name
it?

The bug doesn't need a name. We need to cure the cause. In this case
the programmer made a big mistake and should own up to causing the
problem and fixing it. Hopefully the lesson has been learned and next
time you will take more care when doing something similar.

We like to track defects at work and classify them with names like
"design error", "improper procedure", or "coding problem". I prefer
the term "Bad programmer, bad, bad progammer!" Admit the mistake,
fix it, fix any other potential problems, and learn how to avoid
such mistakes in the future.

David

 

[ankitks]

If destructor code is to be "reused", then most likely the class has a

default "empty" state established by default construction.

In that case one nice technique is to implement a swap() function, which
should never throw but just swap the contents of two objects, and then write

void Note::clear()
{
Note().swap( *this );
}

Voilà, the destructor code has been reused, completely safely!

Sorry, I am little lost. Can you provide some example. So you are
saying that we can do now is
Note::~Note()
{
clear();
}
isn't this will do recursive calling of swap.

Plus, an assignment operator can now reuse the copy constructor (if the
class has a copy constructor):

Note& Note:perator=( Note const& other )
{
Note( other ).swap( *this );
return *this;
}

Plus, that swap operation might come in very handy for other things.

I can see, we are creating new object of note type, with copy
constructor for other. Now we are swaping it contents. So now *this
look like other. But how is this exception safe?

 

[Alf P. Steinbach]

* (e-mail address removed):

Sorry, I am little lost. Can you provide some example.

See above. ;-)

Another example is the idiom for /really/ clearing a std::string or
std::vector, like

template< typename T >
void reallyClear( std::vector<T>& v )
{
std::vector<T>().swap( v );
}

which in practice releases the buffer memory held by v.

And a third example is the idiom for producing a large collection in an
exception safe manner, by clearing the caller's object if and only if
the code succeeds in creating the large collection:

void getManyNumbers( std::vector<int>& result )
{
std::vector<int> numbers;
for( int i = 0; i < 10000; ++i )
{
numbers.push_back( i );
}
// If execution reaches this point all is OK.
// Otherwise, the callers 'result' is unaffected.
numbers.swap( result ); // Won't ever throw.
}

inline std::vector<int> manyNumbers()
{
std::vector<int> result;
getManyNumbers( result );
return result; // Rely on compiler's RVO optimization.
}

So you are
saying that we can do now is
Note::~Note()
{
clear();
}

No. The destructor must do whatever it needs to do to clean up.
clear() uses the destructor logic, by means of C++'s automatic
destructor calls (a temporary object is created and destroyed), and not
the other way around.

isn't this will do recursive calling of swap.
Yes.



I can see, we are creating new object of note type, with copy
constructor for other. Now we are swaping it contents. So now *this
look like other. But how is this exception safe?

The constructor call will either succeed (no exception), or fail with an
exception in which case there's no partially assigned invalid-state
object left behind: there's nothing left behind. If the constructor
call succeeds, swap is called, but is guaranteed to not throw. We say
that swap offers the "no-throw exception guarantee".

From this perspective swap is a more fundamental operation than assignment.

Since most or all assignable standard library classes offer such a swap
operation (I haven't checked whether absolutely all do), this is a very
useful idiom -- one might say that those who put in place all those
swap operations in the standard library, knew what they were about...

In the other direction, I recently had the displeasure of working on a
project where the copy constructor generally was implemented in terms of
the assignment operator, which in turn was implemented in terms of an
Assign() function, which was implemented in terms of MemberWiseCopy,
which should copy only the data members added in the class it was
defined, and this was done for all objects, in particular for those that
should really not be copyable (no distinction was made).

One might say that the architect there did not know what he was about.

 

[ankitks]

Thanks Alf.