Fix #14717 (FP: same exprId for different expressions leads to wrong redundantAssignment)

[danmar]

No description provided.

[danmar]

The problem I saw was that followAllReferences() looked at ternary operator operands. The second operand was "1" for both expressions and then the parent "/" got the same exprId. I have the feeling we have tweaked some handling for ternary operators recently. But very strange that bisect was pointing at other commits..

[firewave]

But very strange that bisect was pointing at other commits..

As mentioned in the comment Visual Studio builds are non-deterministic and somehow Linux builds have consistent results but they might flip with each commit. So regular bisecting is completely impossible.

[firewave]

The fix feels too intrusive for such an subtle issue.

Looking at followingAllReferencesInternal() there is std::set<ReferenceToken, ReferenceTokenLess> result;. As that contains a pointer I wonder if this is causing the issue as the order might be unstable. And as all other cases only return a single result this is the only case where it could differ. I will give this a spin in Visual Studio later.

[firewave]

I have the feeling we have tweaked some handling for ternary operators recently

It wasn't that recently - the issue was introduced in the 2.19 dev cycle.

[firewave]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

[danmar]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

I am not sure I understand the debug output in comment:12 but doesn't it say that the operand1 for the > token is 1, And this expression 1 will get exprId 3. It's OK that all expressions 1 everywhere gets the same exprId 3 right?

[firewave]

Looking at followingAllReferencesInternal() there is std::set<ReferenceToken, ReferenceTokenLess> result;. As that contains a pointer I wonder if this is causing the issue as the order might be unstable.

That is actually an issue.

correct:

op1 - b - 2
op2 - 1 - 3
op2 - [ - 5
? - astParent: = - key.operand1: 2 (b) - key.operand2: 5 (?) - not found - exprId: 9

incorrect:

op1 - b - 2
op2 - [ - 5
op2 - 1 - 3
? - astParent: = - key.operand1: 2 (b) - key.operand2: 3 (?) - not found - exprId: 9

[firewave]

That is actually an issue.

setParentExprId() breaks after the first results entry from followAllReferences() and since that doesn't have a stable order when it returns more than one results that will lead to the non-determinism.

That needs to be fixed inside followAllReferences() by using stable sorting via ReferenceTokenLess (using a pointer for sorting is always bad). The location comes to mind assomething fixed but I have no idea if that would work out with macros.

[firewave]

And this doesn't seem to address the potentially duplicated exprId pointed out in https://trac.cppcheck.net/ticket/14717#comment:12.

That appears to be a non-issue. The logging was just confusing as I was printing the op the exprId was looked up from and not the actual token belonging to the exprId.

[danmar]

The location comes to mind assomething fixed but I have no idea if that would work out with macros.

I am not sure if the Token::index() has been assigned yet. but if it has that would be more stable. The location will not work well there are many simplifications that will not give tokens unique locations.

No matter if we make the order fixed or not we need to ensure that we don't take the expression ids of the ternary operator operands..

When a ternary operator expression is x?y:z then it seems OK to me that followAllReferences can return both y and z. But neither of these results are OK to use here in this context when setting the expression id for the ? or its parent. An alternative fix could be to skip ref.token if its parent is a :

[firewave]

No matter if we make the order fixed or not we need to ensure that we don't take the expression ids of the ternary operator operands..

As a first step we need to make the order stable regardless of the ternary issues.

[danmar]

An alternative fix could be to skip ref.token if its parent is a :

I don't think that suggestion would work well.

As a first step we need to make the order stable regardless of the ternary issues.

It's not clear to me why? the important thing is that we pick the proper ref.token right? if the ternary operator says x?y:z then picking y or z are both wrong. There is no strong reason to pick the first result from followAllReferences.

[danmar]

As a first step we need to make the order stable regardless of the ternary issues.

My intention with my fix is that the order does not matter. My goal was to pick the expression id from the same ref token no matter if refs are order (ref1,ref2) or (ref2,ref1).

[pfultz2]

This could just be a function since it does capture anything.

[danmar]

sure I will fix