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();
}
The Geek Shall Inherit The Earth 
November 5, 2011 at 03:27
He he.. For a second there, I was wondering what’s the big deal! 😉
November 5, 2011 at 11:06
Is a typical Planet KDE reader supposed to understand this?
November 5, 2011 at 12:40
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.
November 6, 2011 at 07:58
The GCC option “-Wshadow” should warn of such errorss
November 5, 2011 at 12:40
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.
November 5, 2011 at 12:43
The destructor did nothing, as the local declaration of that list hid the class member with the same name.
November 5, 2011 at 18:30
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.
November 5, 2011 at 12:20
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 ?
November 5, 2011 at 22:07
Wow. Now we’re into deep discussion. I like that. 😀
November 5, 2011 at 12:26
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. 😉
November 5, 2011 at 12:59
I see a variable definition deleted, and that same variable used the line after.
November 5, 2011 at 13:04
Well, I can see what’s wrong, but I think it would have been funnier to see the patch that introduced the error 🙂
November 5, 2011 at 14:29
omfg
November 5, 2011 at 16:23
Without having touched C++ for over a decade I would guess that it’s futile to iterate over an empty list.
November 5, 2011 at 17:22
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?
November 5, 2011 at 18:02
@usetah: yes. m_ implies it’s a member variable and the local variable is shadowing that one presumably. programming 102
November 5, 2011 at 18:29
I don’t get it.
November 5, 2011 at 22:03
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.
November 5, 2011 at 22:32
No Problem 😉 Actually after reading the comments here, I understand it better. It made me smile ^^
November 5, 2011 at 18:34
I assume that declaring a “m_xxx” variable on the stack shadowed a member variable. The loop was basically iterating over an empty list.
November 5, 2011 at 20:07
I guess it didn’t crash, but silently failed destroying the actual objects while in fact not doing any iteration over the empty list?
November 5, 2011 at 22:06
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.
November 5, 2011 at 20:25
I’ve done that before!
Normally I add it for testing, then just forget completely
November 5, 2011 at 21:02
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!
November 5, 2011 at 21:23
@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.
November 5, 2011 at 22:00
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.
November 5, 2011 at 22:24
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?
November 6, 2011 at 00:03
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 🙂
November 6, 2011 at 01:33
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…
November 6, 2011 at 13:23
You can make GCC give a warning in cases like this one.
Just give the parameter -Wshadow to gcc/g++
November 6, 2011 at 05:15
ROFL
November 6, 2011 at 12:04
Use -Wshadow.
November 6, 2011 at 13:19
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..
November 7, 2011 at 08:59
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++.
November 7, 2011 at 09:59
> 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.