Fix #8266 identicalConditionAfterEarlyExit variable modified in if-clause

[chrchr-github]

For discussion.
This reinstates some warnings associated with #9914, which were specifically suppressed at one point. However, it sounds like the warnings are now OK again?

[pfultz2]

Why are we moving to the lhs side of logical ops? Wont this miss the parent function call? This seems like it would jump to g in the expression f(g(x) || y) instead of f

[chrchr-github]

The example looks something like if (g(b) || b), and g(b) was not considered so far.

[pfultz2]

Ah yea but you need to check isVariableChanged on the first b and not the second b. The first b should return true and the second b should return false.

[pfultz2]

Whats the reason to pass ptok->next()?

[pfultz2]

Dont you need to check if the expression is changed before the secondCondtion not at the secondCondition?

Above at line 663, we check if the variables are changed in the second condition. It should probably just be changed to just use isExpressionChanged(cond1, condStartToken, condEndToken, 0, false, mSettings, mTokenizer->isCPP()).

[chrchr-github]

isVariableChanged seems to make some unstated assumptions about the code that it's operating on, which are not true for the example in #8266. I just made the minimal changes necessary to get rid of the FP (and not crash on other code). Are ftok and ptok supposed to point to function and parameters, respectively?
I'll probably look at it again next week.

[pfultz2]

isVariableChanged seems to make some unstated assumptions about the code that it's operating on, which are not true for the example in #8266.

What assumptions are you talking about? isVariableChanged should return true for b in g(&b).

Are ftok and ptok supposed to point to function and parameters, respectively?

Yes, mostly. Although, it seems like we should remove this ptok adjustment and move the logic into isVariableChangedByFunctionCall. For better context, this was added in bb52a63c4e0481f5693d83770d29088cef70ea2a when const variable check was added.

[chrchr-github]

Back to the unmodified code, with the test case

void f(bool b) {
    if (b)
        return;
    if (g(&b) || b)
        return;
}

Under which circumstances is isExpressionChangedAt(tok, tok2, tok->valueType() ? tok->valueType()->pointer : 0, global, settings, cpp, depth) supposed to return true, assuming that tok points to b in if (b)? E.g. for tok2 = g, (, &, or b?

[pfultz2]

Under which circumstances is isExpressionChangedAt(tok, tok2, tok->valueType() ? tok->valueType()->pointer : 0, global, settings, cpp, depth) supposed to return true

Are you referring to line 657? That is only checking for changes before the if of the second condition.

At line 663, it checks if the second condition modifies the variable or expression. Of course, its quite in adequate and should just use isExpressionChanged(cond1, condStartToken, condEndToken, 0, false, mSettings, mTokenizer->isCPP()).

assuming that tok points to b in if (b)? E.g. for tok2 = g, (, &, or b?

It should return true when tok2 is equal to b in g(&b), but the visitAstNodes will not traverse that deep. It only checks the children of || or &&.

[chrchr-github]

Using isExpressionChanged() seems to be the right fix, thanks for the suggestion. The problem was not were I thought it was.
Edit: unrelated issue removed

[chrchr-github]

Is this OK now?

[danmar]

It looks good to me. If @pfultz2 has no complaint I think we should merge it.

[pfultz2]

LGTM