Fix #10704 FN redundantCopyLocalConst#4115
Conversation
| continue; | ||
|
|
||
| std::string name = tok->strAt(1); | ||
| const std::string& name = tok->strAt(1); |
There was a problem hiding this comment.
This and most (all?) of the other changes seem a bit dangerous. We need to be sure that the owner is const and/or only const member functions are being called. But this holds only true for the moment. If the code is changed afterwards that would actually issues.
It is true that the code could have been written like this in the first place but that was either intentional or erroneous by the coder and not suggested by a tool perceived as "trustworthy".
I did such overzealous changes in #4034 which lead to crashes in the tests.
The example in the ticket which inspired this is valid but there is only a single usage and the call there could be inlined in the single usage and not variable is even necessary.
There was a problem hiding this comment.
I agree that this warning can be dangerous, see the suppression I had to add. On the other hand, the check would have already fired had the local variables been declared as const. That is just a heuristic though and does not indicate whether using a reference is actually safe or not.
There was a problem hiding this comment.
On the other hand, the check would have already fired had the local variables been declared as const.
I knew there was an existing case where we suggest this. I just couldn't remember. So I guess this is fine.
There was a problem hiding this comment.
I agree that this feels a bit dangerous. Tokens are deleted in this function.
Are you sure there is no path where the tok will be deleted before name is accessed?
There was a problem hiding this comment.
name is only read at L2367, deletions start at L2386. So this should be safe.
There was a problem hiding this comment.
But would cppcheck detect the issue if the code was changed in the future to read name after the token was deleted?
At least using a copy this will be more resilient to changes in the future.
There was a problem hiding this comment.
No, we would not detect that, see the suppression that I added elsewhere. I don't think the checker has many clues here to decide if it should warn or not.
|
Blocked by https://trac.cppcheck.net/ticket/11083 for the moment. |
| continue; | ||
|
|
||
| std::string name = tok->strAt(1); | ||
| const std::string& name = tok->strAt(1); |
There was a problem hiding this comment.
I agree that this feels a bit dangerous. Tokens are deleted in this function.
Are you sure there is no path where the tok will be deleted before name is accessed?
|
It seems the checker needs to be more robust first. The suppression shows a dangerous false positive that could cause bugs. |
|
Hmm, there are only three true positive test cases so far, and they all concern static (member) variables. I agree that we cannot warn if the underlying resource might be modified in any way. |
|
Any more feedback? |
4 participants
No description provided.