Fix issue 9133: Invalid iterator; vector::push_back, functions#3008
Conversation
|
|
||
| int getArgumentPos(const Variable* var, const Function* f) | ||
| { | ||
| auto arg_it = std::find_if(f->argumentList.begin(), f->argumentList.end(), [&](const Variable& v) { |
There was a problem hiding this comment.
I wonder if it would be a good idea to capture var by value not by reference.
There was a problem hiding this comment.
Why? It seems simpler and faster to capture by ref as it has lexical scoping and avoids creating copies.
There was a problem hiding this comment.
the motivation would be if we get some const safety. if it doesn't have a performance penalty.. that would be good. for a trivial example I made the assembler output from gcc is identical.
There was a problem hiding this comment.
The variables are already declared const.
There was a problem hiding this comment.
I assume that in the lambda you could write var=nullptr; and that will happily compile. well.. maybe that compiles happily with a = also?
There was a problem hiding this comment.
Yea, it can be modified with var=nullptr. I can make the pointer const.
| ErrorPath errorPath; | ||
| void add(const std::vector<Reference>& refs) | ||
| { | ||
| for (const Reference r : refs) { |
| auto it = invalidMethods.find(f); | ||
| if (it != invalidMethods.end()) { | ||
| std::vector<Info::Reference> refs = it->second.invalidTokens(); | ||
| std::copy_if(refs.begin(), refs.end(), std::back_inserter(result), [&](const Info::Reference& r) { |
There was a problem hiding this comment.
I wonder if it would be a good idea to capture dependsOnThis by value. Does it actually capture everything in the context or are compilers smart enough and captures only dependsOnThis.
There was a problem hiding this comment.
It only captures the variables used, but compilers can optimize it further and just capture the stack pointer instead of each variable used. Usually, its inlined so there is no copy(or copy to pointer) or function call(see this example).
There was a problem hiding this comment.
It's good practice to limit the amount of variable captured.
| return true; | ||
| }); | ||
| std::vector<const Token*> args = getArguments(tok); | ||
| for (Info::Reference& r : result) { |
There was a problem hiding this comment.
so I guess you will fix something here before we merge it?
There was a problem hiding this comment.
What needs to be fixed?
There was a problem hiding this comment.
I wonder .. if result is always empty. Is the loop redundant? If the loop is redundant then remove it.. but if it is not then please tell me how result could be nonempty..
There was a problem hiding this comment.
Its not always empty. The copy_if fills the result.
There was a problem hiding this comment.
ok thanks for that clarification I somehow missed it.
danmar
left a comment
There was a problem hiding this comment.
In my opinion this can be merged.. if the merge conflict is solved.
|
Is there any more changes needed? |
No description provided.