Uh oh!
There was an error while loading. Please reload this page.
feat(unused_var): analyze global variables inside function body - #2944
Conversation
danmar
commented
Dec 14, 2020
Thanks! I will look this evening.. |
danmar
left a comment
There was a problem hiding this comment.
small nits.. I will test it out a bit..
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
danmar
left a comment
There was a problem hiding this comment.
hmm I have tried it out and it works better than I thought!
| // check if pointer to global variable assigned to another variable (another_var = &global_var) | ||
| if (Token::simpleMatch(bodyToken->tokAt(-1), "&") && Token::simpleMatch(bodyToken->tokAt(-2), "=")) { | ||
| const Token* assigned_var_token = bodyToken->tokAt(-3); | ||
| if (assigned_var_token && assigned_var_token->variable()) |
There was a problem hiding this comment.
if this condition is true then you don't care that the address is used somehow.. would probably be safer to return false then.
There was a problem hiding this comment.
As I see, isVariableChanged() works fine enough with "pointer-to-global" variables. I prefer to leave it as is, if you not insist.
Do you see any test cases which will force this code work not as expected?
There was a problem hiding this comment.
I think it works well.. right now I am only worried about the todo test case. Do you know it it's complex to fix that?
There was a problem hiding this comment.
Probably, it should be not so complex. But it can affect existing tests and I will need some time to figure out with them.
Also I can not estimate a free time amount I will be able to put on this work in the nearest time. =)
How about this one - I will return false for now and update astutils.cpp and this piece of code in next PR?
There was a problem hiding this comment.
It's fine to make a partial fix.. and finish later. but then I feel more comfortable if there are no false positives in the meantime. So well.. could you return false if there is a global array/pointer variable.
| @@ -1623,15 +1623,16 @@ bool CheckUnusedVar::isFunctionWithoutSideEffects(const Function& func, const To | |||
| if (bodyVariable->isGlobal() || | |||
| (std::find(pointersToGlobals.begin(), pointersToGlobals.end(), bodyVariable) != pointersToGlobals.end())) | |||
There was a problem hiding this comment.
Now we can use pointersToGlobals.find(bodyVariable) != pointersToGlobals.end() instead here.
danmar
commented
Dec 19, 2020
Thanks! |
miltolstoy
commented
Dec 19, 2020
Welcome. |
Before this PR, any global variable found in function body marked this function as side-effect one. Analyzing global variables more precisely from now.
Any help with missed test cases will be highly appreciated.