Fix issue 9935: FP: knownConditionTrueFalse value flow doesn't account for virtual functions

[pfultz2]

No description provided.

[amai2012]

What it be useful to keep the current message but make it inconclusive?

[pfultz2]

What it be useful to keep the current message but make it inconclusive?

Yea we can do that.

[amai2012]

@danmar Would you prefer the message to be removed or made inconclusive?

[danmar]

I do not have a strong opinion about inconclusive. Traditionally I have not made that decision. If anybody wants to experiment with that feel free; it would be interesting to know what the noise level is according to daca@home. If the noise level is close to 100% then it's not okay. I am not sure what noise level is acceptable I imagine something like 50%.. do you have some opinions about what noise level we could accept for inconclusive warnings?

[amai2012]

It depends...
In case the message is about a severe issue I'd be willing to accept even a higher noise ratio >50%.
For a style issue or potential bug ("it might be wrong...") I would like to ignore messages with 50% false positive rate.

But can we disable a message only if it's inconclusive!? I think there is only a global switch to enable inconclusive messages at all.

[danmar]

But can we disable a message only if it's inconclusive!?

Not yet.. unless it will get a separate id. It would be pretty interesting with a separate id to be able to evaluate how well it works in daca@home. I suggest knownConditionTrueFalseVirtualCall or something like that.

[danmar]

In case the message is about a sever issue I'd be willing to accept even a higher noise ratio >50%.
For a style issue or potential bug ("it might be wrong...") I would like to ignore messages with 50% false positive rate.

Feel free to open a PR to tweak our philosophy.md document. I don't have a strong opinion, whatever the team and users like is fine for me..

[danmar]

@pfultz2 I think this is fine to merge as is.. do you want that I merge it now or do you want to make the message inconclusive first? (no hurry)

[pfultz2]

I think this is fine to merge as is.. do you want that I merge it now or do you want to make the message inconclusive first? (no hurry)

I think it can be merged in. It will take more refactoring to change it to inconclusive. However, this will just change valueflow to inconclusive. There wont be an inconclusive message in this case because we dont know if an inconclusive value is known or possible.