Are you sure?
Can you post the entire function?
Are you sure?
Can you post the entire function?
Here's the function. It's part of a big program, and I can't isolate the problem easily. But the fact is that when I take out the "QString path=path_in;" line and replace "path_in" by "path" in the function arguments, I get a serious problem.
I think this could be a bug with MinGW g++ ? !!!
Qt Code:
if (!nodes.contains(path)) return; if (!nodes[path].needs_to_be_updated) return; nodes[path].needs_to_be_updated=false; nodes[path].children.clear(); QList<HaiQTag> tags=core->tagManager->byPath(path); QStringList search_paths=include_search_paths; QStringList included_paths; for (int j=0; j<tags.count(); j++) { if (tags[j].tagtype=="pound_include") { if (!holdstr.isEmpty()) included_paths << holdstr; } } for (int j=0; j<included_paths.count(); j++) { if (!nodes.contains(path2)) { core->fileManager->addFile(path2); visible_files_node newnode; newnode.path=path2; newnode.needs_to_be_updated=true; nodes[path2]=newnode; } if (!nodes[path].children.contains(path2)) { nodes[path].children << path2; } if (nodes[path2].needs_to_be_updated) update_node(path2,false); //Here's the recursive part where the value of path becomes corrupted } if (do_emit_signal) emit updated(); }To copy to clipboard, switch view to plain text mode
But it's path2 not path.
What happens if you change your code to:?Qt Code:
if (nodes[path2].needs_to_be_updated) { static int level = 0; qDebug() << "Before" << ++level << path << in_path; update_node(path2,false); //Here's the recursive part where the value of path becomes corrupted qDebug() << "After " << level-- << path << in_path; }To copy to clipboard, switch view to plain text mode
Thanks, Jacek. I put in those type of debug strings as you suggested (code is at the bottom)... and now I'm really starting to be worried of a fundamental bug in the compiler.
The "path" variable changes right in the middle of the function, during a call to a completely different function. I should mention that the "update_node()" function is in a plugin library, and the implementation of the offending "addFile()" is in the main application.
Here's the log that I get out, using the two scenarios. Notice that the "path" variable changes right between debug 5 and debug 6.
Qt Code:
LOG OUTPUT SCENARIO #1 ... 0 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::4: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::5: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::6: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::7: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::8: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::9: "c:/qt/4.3.2/include/qtgui/qwidget" 1 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::11: "c:/qt/4.3.2/include/qtgui/qapplication.h" 0 update_node:::::10: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::11: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::4: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::5: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::6: "c:/qt/4.3.2/include/qtcore/qdebug" 0 update_node:::::7: "c:/qt/4.3.2/include/qtcore/qdebug" 0 update_node:::::8: "c:/qt/4.3.2/include/qtcore/qdebug" 0 update_node:::::9: "c:/qt/4.3.2/include/qtcore/qdebug" ... LOG OUTPUT SCENARIO #2 ... 0 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::4: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::5: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::6: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::7: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::8: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::9: "c:/qt/4.3.2/include/qtgui/qapplication" 1 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qapplication.h" 1 update_node:::::11: "c:/qt/4.3.2/include/qtgui/qapplication.h" 0 update_node:::::10: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::11: "c:/qt/4.3.2/include/qtgui/qapplication" 0 update_node:::::1: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::2: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::3: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::4: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::5: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::6: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::7: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::8: "c:/qt/4.3.2/include/qtgui/qwidget" 0 update_node:::::9: "c:/qt/4.3.2/include/qtgui/qwidget" ...To copy to clipboard, switch view to plain text mode
And here's the code that produced these two (only difference is in the top three lines):
CODE FOR SCENARIO #1
Qt Code:
static int level=0; //QString path=path_in; //apparently this is important :( qDebug() << level << "update_node:::::1:" <<path; if (!nodes.contains(path)) return; if (!nodes[path].needs_to_be_updated) return; nodes[path].needs_to_be_updated=false; nodes[path].children.clear(); QList<HaiQTag> tags=core->tagManager->byPath(path); emit file_tags_updated(path,tags); QStringList search_paths=include_search_paths; QStringList included_paths; qDebug() << level << "update_node:::::2:" <<path; for (int j=0; j<tags.count(); j++) { if (tags[j].tagtype=="pound_include") { if (!holdstr.isEmpty()) included_paths << holdstr; } } qDebug() << level << "update_node:::::3:" <<path; for (int j=0; j<included_paths.count(); j++) { qDebug() << level << "update_node:::::4:" <<path; if (!nodes.contains(path2)) { qDebug() << level << "update_node:::::5:" <<path; core->fileManager->addFile(path2); qDebug() << level << "update_node:::::6:" <<path; visible_files_node newnode; newnode.path=path2; newnode.needs_to_be_updated=true; nodes[path2]=newnode; } qDebug() << level << "update_node:::::7:" <<path; if ((!nodes[path].children.contains(path2))&&(path!=path2)) { nodes[path].children << path2; } qDebug() << level << "update_node:::::8:" <<path; if (nodes[path2].needs_to_be_updated) { qDebug() << level << "update_node:::::9:" <<path; level++; update_node(path2); level--; qDebug() << level << "update_node:::::10:" <<path; } } qDebug() << level << "update_node:::::11:" << path; }To copy to clipboard, switch view to plain text mode
CODE FOR SCENARIO #2
Qt Code:
static int level=0; qDebug() << level << "update_node:::::1:" <<path; if (!nodes.contains(path)) return; if (!nodes[path].needs_to_be_updated) return; nodes[path].needs_to_be_updated=false; nodes[path].children.clear(); QList<HaiQTag> tags=core->tagManager->byPath(path); emit file_tags_updated(path,tags); QStringList search_paths=include_search_paths; QStringList included_paths; qDebug() << level << "update_node:::::2:" <<path; for (int j=0; j<tags.count(); j++) { if (tags[j].tagtype=="pound_include") { if (!holdstr.isEmpty()) included_paths << holdstr; } } qDebug() << level << "update_node:::::3:" <<path; for (int j=0; j<included_paths.count(); j++) { qDebug() << level << "update_node:::::4:" <<path; if (!nodes.contains(path2)) { qDebug() << level << "update_node:::::5:" <<path; core->fileManager->addFile(path2); qDebug() << level << "update_node:::::6:" <<path; visible_files_node newnode; newnode.path=path2; newnode.needs_to_be_updated=true; nodes[path2]=newnode; } qDebug() << level << "update_node:::::7:" <<path; if ((!nodes[path].children.contains(path2))&&(path!=path2)) { nodes[path].children << path2; } qDebug() << level << "update_node:::::8:" <<path; if (nodes[path2].needs_to_be_updated) { qDebug() << level << "update_node:::::9:" <<path; level++; update_node(path2); level--; qDebug() << level << "update_node:::::10:" <<path; } } qDebug() << level << "update_node:::::11:" << path; }To copy to clipboard, switch view to plain text mode
and here is the declaration of addFile...
Qt Code:
To copy to clipboard, switch view to plain text mode
This means that we have to take a look at this completely different function. It isn't time for worrying yet.
Probably your code does something similar to this:
Qt Code:
#include <iostream> int z = 0; void bar( int y ) { z = y; } void foo( const int & x ) { std::cerr << x << std::endl; bar( 1 ); std::cerr << x << std::endl; } int main() { foo( z ); return 0; }To copy to clipboard, switch view to plain text mode
Gopala Krishna (20th November 2007), magland (17th November 2007)
Ok, I see what you are saying. Now I see the danger of passing things by reference (even if declared const). The advantage is that the parameter does not need to be copied, but the tradeoff is that you need to worry about whether the variable is changed part way through the execution of the function - not within the function but elsewhere.
This is probably what was happening to me. But I will never know, because I've restructured my program to avoid the recursion (I'm now using a queuing system to do the same task).
Based on a recent recommendation on this forum I changed my coding habits and use "const QString &" everywhere instead of "QString".... but now I see that I need to be more careful. Do you have a recommendation of what precautions I need to take with this?
Thanks!
Gopala Krishna (20th November 2007)
It's recommended to pass objects by reference to avoid copying, but on the other hand in Qt it's cheap to copy implicitly shared classes.
I was thinking about this for few days and I couldn't come up with any rules. You situation is a bit similar to: The argument is passed as reference, but it must be copied when QVector needs to reallocate data. You can easily identify such situation in a single data structure, but the problem arises when the data is passed between many classes. But I would simply classify that as one of those Bad Things(tm) that might happen when you use global variables (or you have cycles in call graph).
Thanks Jacek. I agree with you.
After finding a number of subtle problems caused by passing by reference (in addition to the one I reported) I have concluded that it is important to NEVER pass arguments by reference UNLESS you can guarantee that the function does not access global variables, or "have cycles in the call graph" that may end up modifying the input variable. It is just not worth the danger! There's nothing worse than problems that occur mysteriously once in every 1000 calls.
Of course, passing by reference is good, if you can ensure such things. I don't use global variables, but I do use a model/view framework which inevitably will have some cycles or recursive calls. Therefore, I need to be careful about it.
Very cool observation jacek :-)
So basically in the code posted by magland, it seems that the parameter passed is a class member variable(as there is an emit statement, the func should be a class member) which is modified somewhere may be in a slot.
The actual answer to whether the reference form should be used or not is a dependent question. It depends on whether you modify the parameter sent outside the function when the function with const reference is not yet returned.
Anyway, many thanks Jacek as well as magland for exposing such a kind of potential bug.
A lesson learnt well in the right time![]()
The biggest difference between time and space is that you can't reuse time.
-- Merrick Furst
Bookmarks