Lost for words

November 4, 2011

commit 740673c11aee9762987e3a205443ce1dec11aee8
Author: Stefan Majewsky <majewsky@gmx.net>
Date:   Sat Nov 5 00:11:37 2011 +0100

    lolwut?

diff --git a/tagaro/interface/board.cpp b/tagaro/interface/board.cpp
index 199c007..17dbdfc 100644
--- a/tagaro/interface/board.cpp
+++ b/tagaro/interface/board.cpp
@@ -42,7 +42,6 @@ Tagaro::Board::~Board()
 
 Tagaro::Board::Private::~Private()
 {
-       QList<Tagaro::SpriteObjectItem*> m_items;
        for(QList<Tagaro::SpriteObjectItem*>::const_iterator a = m_items.constBegin(); a != m_items.constEnd(); ++a)
                (*a)->d->unsetBoard();
 }

About these ads

35 Responses to “Lost for words”

  1. Syam Says:

    He he.. For a second there, I was wondering what’s the big deal! ;-)

  2. uetsah Says:

    Is a typical Planet KDE reader supposed to understand this?

    • Morten Says:

      Someone who knows a little C++ should understand it :)
      Tagaro::Board::Private::~Private() is a decontructor, that’s a function that is called when an object is deleted.

      Without knowing anything about the code this is from, I’m guessing that m_items is a member of the class Tagaro::Board::Private and that the for-loop is meant to clear each element in the member variable m_items.
      But since someone has instantiated a local object with the same name, the member m_list isn’t cleared in the deconstructor.
      Which leads to a memory leak.

      I could of cause be wrong, it’s been a while since I worked with C++.

      Personally I like these kind of posts and I hope to see more. Often I learn something from them.

    • Paul Eggleton Says:

      A typical C++ programmer ought to. That’s pretty crazy…

      To explain – this is a destructor, where variables are supposed to be freed. Instead of freeing the contents of what should be an object-level list m_items, the original author creates a local m_items that masks the object-level list then “iterates” over it (of course it will always be empty).

      Trouble is, errors like this can be hard to catch; no compiler will complain about this AFAIK.


    • The destructor did nothing, as the local declaration of that list hid the class member with the same name.

    • Johan Ouwerkerk Says:

      C++ programmers probably would. It’s a combination of local variable names hiding member fields and creating stuff (what the removed line did) where you are supposed to delete it (the destructor). That is utterly nonsensical, hence the commit message.

  3. arkascha Says:

    LOL, yes, this is a great one :-)
    Certainly, an insider meant for developers, but you cannot really add a text to this.

    And then …
    Isnt it things you dont understand at the first glance that make you curious ? That make you want to learn ? That make you understand on a deeper level in the end ?

  4. tuxu Says:

    As this is the destructor of the class, it should free the memory of its members or at least do something before its destruction. Here a new variable m_items is created (empty, of course), which brings the real member Tagaro::Board::Private::m_items out of scope. In the end, the empty list is traversed and nothing happens. A bug easy to create and hard to spot. ;-)

  5. Gallaecio Says:

    I see a variable definition deleted, and that same variable used the line after.

  6. Gnud Says:

    Well, I can see what’s wrong, but I think it would have been funnier to see the patch that introduced the error :)

  7. tanghus Says:

    Without having touched C++ for over a decade I would guess that it’s futile to iterate over an empty list.

  8. Patrick Says:

    uetsah: probably ;-)
    To explain:
    objects that begin with m_ (like m_items) are members of a class (in this case the class Tagaro::Board::Private). The line that is removed here (the red one with the – in the beginning) declares this object locally in a function which is a real wtf and which leads to unwanted results (bugs).

    @Stefan: To be honest I have an idea why somebody committed code like that (very likely crashes in unsetBoard()) but more important: why does gcc even accept this?

  9. Sekar Says:

    @usetah: yes. m_ implies it’s a member variable and the local variable is shadowing that one presumably. programming 102

  10. Burke Says:

    I don’t get it.

    • Stefan Majewsky Says:

      You need to be familiar with C++ and coding style to understand this. Kind of an inside joke, yes. But since I banged my head at the desk for minutes because of this bug, I deemed it worth sharing.


  11. I assume that declaring a “m_xxx” variable on the stack shadowed a member variable. The loop was basically iterating over an empty list.

  12. thorGT Says:

    I guess it didn’t crash, but silently failed destroying the actual objects while in fact not doing any iteration over the empty list?

    • Stefan Majewsky Says:

      It was actually causing a crash, because unsetBoard() properly cleans up the mutual relation between the board and its items. Since the items are not disconnected in the board dtor, they will attempt to call the board in their own dtor.


  13. I’ve done that before!
    Normally I add it for testing, then just forget completely

  14. BBroeksema Says:

    Hehe awesome, redefining a variable which looks very much like class member. At least it is very clear that this localy defined list never will contain an item!

  15. MaikB Says:

    @uetsah, Burke

    m_items is most likely the name of a member variable. A member of the class Tagaro::Board::Private. Introducing a local variable with the same name, m_items, will shadow the member variable. In this case the for loop operated on a the local variable rather than the member variable.

  16. Loreia Says:

    uetsah and Burke:

    This is an example of memory leak.

    ~Private() is a class destructor ( a place where you can free allocated memory)

    m_items is a class member variable (at least it should be, that part of class is omitted from above example), that represents a list of pointers (each pointer should be deleted before exiting from class)

    What happened here is a mistake that can easily happen to anyone after coding for hours without a break:

    Red line actually creates a new empty list, and rest of the destructor simply iterates over an empty list (i.e. it does nothing). If red line is omitted, class works as intended.

    This is a beginners mistake that can happen even to experienced coders, all it takes is to be hasty or tired. In my opinion this is not worthy of “wall of shame” type of post. Unless the author of blog is also the author of code, in that case it is nice to show such simple examples of memory related errors.

  17. STiAT Says:

    I looked at this and just wondered why it’s trying to iterate over an empty list until i realized it’s a destructor and it’s a m_ variable ^^.

    Nice one, but shouldn’t the compiler at least put a warning that m_items matches an existing decleration?

  18. [Po]lentino Says:

    Lol so someone named a local variable using the standard convention for naming class variables members ( appending “m_” in front of the var name) ?
    I hope you didnt’t wasted too much of your time with this bug :)

  19. uetsah Says:

    OK, so the obvious question is: Why does C++ allow defining a local variable with the same name as an existing member variable in the first place? Seems like asking for trouble…

  20. AnCoward Says:

    Use -Wshadow.

  21. renoX Says:

    Yep, I agree with uetsah: allowing silent overloading of local variables and member field is insane, and the use of naming conventions to distinguish both helps but is not enough as seen here..
    Of course we’re talking about C++ so insanity is expected..

    • Stefan Majewsky Says:

      The following is also possible:

      void f() { int i = 0; { int i = 1; } }

      The two obvious solutions (unless you want to go the Python way and be drowned by “self.”) are the C++ way (allow shadowing in every nested scope), or the exact opposite (disallow shadowing everywhere). The second alternative is not viable because introducing a new global variable could break existing code.

      C++ is surely complicated, but its design is very coherent once you grasp it. There’s a book by Stroustrup about the design of C++, which is an excellent read for those interested in understanding the design decisions behind C++.

      • renoX Says:

        > The second alternative is not viable because introducing a new global variable could break existing code.

        A false dichotomy, my solution would be: the language definition would require that a compiler should warn of any shadowing (with a switch to disable such warning).
        Any shadowing is a risk.

        As for design, Ada has also a very coherent design, but it’s much better than C++’s design.


Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.