Fragile API and invalid iterators
For the Miniature project I sometimes need to iterate over all graphics items in a QGraphicsScene, or more precisely, all items of a specific parent item, the chess board itself. For that, I use the QGraphicsItem::childItems() API. However, when used with STL-style iterators you have to be careful, since value types of that form can easily break the iterator idiom:
for (QList<QGraphicsItem *>::iterator iter = parent_item->childItems().begin();
iter != parent_item->childItems().end();
++iter)
{
(*iter)->doSth(); // Ooops! Invalid iterator deref'ing!
}
The value-type list that is returned by parent_item->childItems() has not been bound to a variable explicitly, so it is bound to an anonymous variable instead. The compiler will not complain here although the code is borken, from a C++ point of view. Worse yet, "(*iter)->doSth();" might work often enough for you to not see the crashes right away, depending on the compiled code and how the architecture handles memory.
Eventually though it will crash, and according to Murphy this will happen right after the big release. Valgrind of course would have complained about the illegal memory access right away (when run through that part of your code), because the list that we queried in the loop header is invalidated as soon as we enter the loop body, leaving us with an iterator that points into the void. This is C++-specific, in other languages even anonymous variables are only ever cleaned up after leaving the current block context, but in C++ their life time is only guaranteed for the scope of the current expression.
There is one fix and one workaround to that problem. First the fix: let QGraphicsItem::childItems() return a reference to a list member instead, or simply expose the begin/end iterators for the internal data structure directly. Assuming this is not possible, we are left with the workaround - bind the returned list to a variable explicitly, before the iterator is used:
QList<QGraphicsItem *> children = parent_item->childItems();
for (QList<QGraphicsItem *>::iterator iter = children.begin();
iter != children.end();
++iter)
{
(*iter)->doSth(); // Fine!
}
This of course prolongs the life time of the returned list unnecessarily. We only needed it inside the loop, but now it won't be cleaned up until the flow of control leaves the surrounding block context. To finally get the desired behaviour we could limit the surrounding block context:
{
QList<QGraphicsItem *> children = parent_item->childItems();
for (QList<QGraphicsItem *>::iterator iter = children.begin();
iter != children.end();
++iter)
{
(*iter)->doSth(); // Fine!
}
}
Perhaps a cleaner way is to move the loop code in a function of its own instead, although that might not always be feasible, depending on the code inside the loop body.
Another issue might be the need to dynamically cast QGraphicsItems (QGI) to the desired type, once you implemented your custom QGraphicsItem type. This wouldn't be too bad if there wasn't this horrible mix of QGraphicsObjects and QGraphicsItems in Qt 4.6 - some items inherit from QObject (QGraphicsSvgItem), others don't (QGraphicsPixmapItem). So now you also have to decide between dynamic_casts and qobject_casts, how nice ... not! Honestly, it would be easier here to simply forget about qobject_casts, but that might break in subtle ways.
These two issues were enough for me to avoid the QGI::childItems() API whenever possible. One possible replacement can be provided by signals and slots [1]: First, add a slot representing the loop body to your custom QGI type. Second, connect a signal to the instances of your QGI type that would have been iterated in the loop, that is, have a custom signal representing the loop iteration. Then, instead of iterating over the children of the parent item you simply emit the custom signal which triggers the "loop body" execution for each connected custom QGI instance.
[1] requires QObject inheritance for your custom QGraphicsItem type
Your actual problem is that QList performs implicit copy-on-write. The call to childItems() will return a QList that points to a shared instance -- so far, so good. But since you are using the non-const overload of childItems(), the calls to begin() and end() will detach from the shared object and create separate copies (twice!).
So, the comparison of the iterators is already invalid because they are pointing into different containers. And both temporary QList objects go out of scope soon after.
Oh, and you should of course be using std::for_each() and std::mem_fun_ptr()!
:P
Oh, and by the way, unless you actually know that a function returns by reference or proxy, you should always assume that any () involves a temporary of some sort.
Daniel: One can also be wise and just avoid wasting time with debugging the error messages caused by std::for_each() and std::mem_fun_ptr() when not used properly. Only the biggest C++ fanboys use them. All other people stay away - for good reason.
Bah, stop bitching Mathias :)
And std::tr1::mem_fn() alleviates the problem greatly by unifying the syntax.
No Daniel, I won't stop trying to bring you back to reality.
*shrug*
It works just fine in my reality. Except that it's actually std::mem_fun(), not mem_fun_ptr() -- my mistake. Anyway, programmers all over the planet are using this sort of stuff daily. See libsigc++.
In any case, we're kinda off-topic now.
Daniel: Also people all over the world are playing Theremins, but does this fact tell anything about the instrument?
Actually it's quite funny and probably proves my point, that even a die-hard C++ fan like you makes mistakes when it comes to functors. Priceless.
Now you are diverting from rational argument.
I got the *name* wrong. 'nuff said.
Daniel, sorry? I just pointed out that you used an invalid argument to justify your position:
> Anyway, programmers all over the planet are using this sort of stuff daily.
That's simply no valid argument. Well, and actually I never denied that __some__ people are using this C++ feature. Some even love it, but this are the fans. Still I am convinced that a much larger portion of programmers just hates C++ functors. Simply for the fact that none of the common C++ compilers produces readable error messages when you mess up with them. Even you should be able to admit, that analyzing 200+ character long error messages of pure template gliberish is ridicilous and annoying. Well, and that's what you get on the most trivial typos when using C++ functors. That's why I consider them a big annoyance that's usually not worth being used. ESPECIALLY not for loops. Using them in signal handlers that's quite a different topic already.
Mathias, you cannot raise the argumentum ad populum fallacy if you brought it up in the first place.
I'm out now.
@Daniel: "Your actual problem is that QList performs implicit copy-on-write."
I wouldn't call it a problem in this context, COW is nice to have. I explicitly discussed the copy-by-value semantics, and that's what COW degenerates into, for non-const iterators, no?
@Daniel: "... you are using the non-const overload of childItems()"
What do you mean? There is only a const getter for this method.
Anway, I'd be more interested to know why anonymous variables have expression scope. Most often, I probably see them in return statements and I cannot see any disadvantage for a block scope in that case. It must be something else.
Yes, it degenerates to a value copy. The problem is that you are taking two separate value copies.
About the overload -- sorry, my bad. I assumed it was overloaded. Hm, looks like this is one of the few situations in C++ where it makes sense to return a constant object by value. If childItems() would return const QList<>, your code would have worked.
Regarding the scope of temporaries: the way it works seems natural to me, but then again I'm used to it. I guess I could think of legitimate code which would explicitly rely on the current behavior, but I guess that won't convince you much. :-)
In any case, even if temporaries had block scope in C++, the example you presented would still break for the reason stated above.
@Daniel: The anonymous variables' scope is what makes the first dereferencing of the iterator illegal. Granted, the loop would run over the collection boundaries regardless, hence my remark of "fragile API": when combined with iterators this method can be easily used wrong. And both could be avoided by returning a reference instead. (It is even more annoying since all of this is a complete non-issue when used with Java-style iterators.)