The "stealing code from the destructor" bug

Discussion in 'C++' started by gw7rib@aol.com, Oct 21, 2006.

  1. Guest

    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.
     
    , Oct 21, 2006
    #1
    1. Advertising

  2. * :
    > 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::eek: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! :)

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    Alf P. Steinbach, Oct 21, 2006
    #2
    1. Advertising

  3. Doug Guest

    wrote:

    > 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 cock 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
     
    Doug, Oct 21, 2006
    #3
  4. On Sat, 21 Oct 2006, Alf P. Steinbach wrote:
    > * :
    >> 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.


    (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::eek: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
     
    Arthur J. O'Dwyer, Oct 21, 2006
    #4
  5. * Arthur J. O'Dwyer:
    >
    > On Sat, 21 Oct 2006, Alf P. Steinbach wrote:
    >>
    >> Note& Note::eek: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.


    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

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    Alf P. Steinbach, Oct 21, 2006
    #5
  6. David Guest

    Hello,

    On Sat, 21 Oct 2006 09:21:57 UTC, wrote:

    > 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
     
    David, Oct 22, 2006
    #6
  7. Guest

    > 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::eek: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?
     
    , Oct 22, 2006
    #7
  8. * :
    >> 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.


    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.


    >> Plus, an assignment operator can now reuse the copy constructor (if the
    >> class has a copy constructor):
    >>
    >> Note& Note::eek: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?


    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.

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
     
    Alf P. Steinbach, Oct 22, 2006
    #8
  9. Guest

    Thanks Alf.
     
    , Oct 22, 2006
    #9
    1. Advertising

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

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. John

    Re: BUG? OR NOT A BUG?

    John, Sep 20, 2005, in forum: ASP .Net
    Replies:
    2
    Views:
    574
  2. RedEye
    Replies:
    2
    Views:
    596
    Jason Kester
    Dec 13, 2005
  3. Michel Joly de Lotbiniere

    Bug Parade Bug 4953793

    Michel Joly de Lotbiniere, Nov 30, 2003, in forum: Java
    Replies:
    4
    Views:
    655
    Michel
    Dec 2, 2003
  4. DarkSpy
    Replies:
    4
    Views:
    899
    tom_usenet
    Jun 27, 2003
  5. Steve Holden
    Replies:
    1
    Views:
    409
    Behrang Dadsetan
    Jul 2, 2003
Loading...

Share This Page