Fix false negatives in checkBitwiseOnBoolean by tgnottingham · Pull Request #2220 · cppcheck-opensource/cppcheck

tgnottingham · 2019-09-26T05:32:48Z

Use AST-based tests in favor of token-based tests for greater coverage.

danmar · 2019-09-26T08:11:34Z

Use AST-based tests in favor of token-based tests for greater coverage.

Thanks, that is way better!

amai2012 · 2019-09-26T18:42:32Z

Now the error message lacks some detail, the variable name: Could some more details be added again?

tgnottingham · 2019-09-27T06:24:03Z

@amai2012, you're right. Though, since the boolean in question could be a long expression, it may be unpleasant to look at.

This might be better handled as a separate issue, in that there are many checks which could print more context, and we could enhance reporting in general. I'll follow up on this.

amai2012 · 2019-09-27T06:52:29Z

@danmar IIRC recently the column attribute was added. That is not yet mandatory in the message? Or just missing in this output?

danmar · 2019-09-27T11:18:24Z

The column is not written by default in the tests. But it should be written in the normal output.

amai2012 · 2019-10-04T07:24:04Z

@danmar Could you please trigger a re-run? I could not find the reason for travis failure...
Do you think it's ok to accept the lack of details in the message and cure it later? Usually we should try to avoid regressions.

versat · 2019-10-04T07:24:15Z

Cppcheck executed by Travis now finds several suspicious places in Cppcheck itself:
IMO & is used by intention here since diag (when not specified otherwise) does insert the token. So the function has side effects and with && the second condition would maybe not tested and thus the token not inserted.
IMHO this could and should be written in another way that is better readable and does not mislead static analyzers (i saw that PVS Studio also complains here).

lib/checkcondition.cpp:473:20: warning: Boolean is used in bitwise operation. Did you mean '&&'? [bitwiseOnBoolean]
    if (diag(tok1) & diag(tok2))
                   ^
lib/checkcondition.cpp:536:22: warning: Boolean is used in bitwise operation. Did you mean '&&'? [bitwiseOnBoolean]
    if (diag(ifCond) & diag(elseIfCond))
                     ^
lib/checkcondition.cpp:801:20: warning: Boolean is used in bitwise operation. Did you mean '&&'? [bitwiseOnBoolean]
    if (diag(tok1) & diag(tok2))
                   ^
lib/checkcondition.cpp:816:20: warning: Boolean is used in bitwise operation. Did you mean '&&'? [bitwiseOnBoolean]
    if (diag(tok1) & diag(tok2))
                   ^
lib/checkcondition.cpp:831:21: warning: Boolean is used in bitwise operation. Did you mean '&&'? [bitwiseOnBoolean]
    if (diag(cond1) & diag(cond2))
                    ^

The code referenced in the following warnings looks like it was written using bitwise operations to be shorter, maybe to be better readable (not sure if it really is the case). IMHO when explicitly using boolean types then no bitwise operations should be used. Otherwise human or machine readers could be mislead. If bitwise operations should be used, the variable types could be changed to integers.

lib/library.cpp:675:44: warning: Boolean is used in bitwise operation. Did you mean '||'? [bitwiseOnBoolean]
                            error |= range | (*(p+1) == '.');
                                           ^
lib/library.cpp:685:46: warning: Boolean is used in bitwise operation. Did you mean '||'? [bitwiseOnBoolean]
                            error |= has_dot | (!std::isdigit(*(p+1)));
                                             ^

The code referenced in the following warning can be clearly optimized. I see no need to use a bitwise operator here.

lib/valueflow.cpp:1014:68: warning: Boolean is used in bitwise operation. Did you mean '||'? [bitwiseOnBoolean]
                    result.setInconclusive(value1.isInconclusive() | value2.isInconclusive());
                                                                   ^

IMHO all the detected issues are more or less true positives that uncover so-called code smells that could be changed to make the code better readable.
@danmar What do you think?

amai2012 · 2019-10-04T07:47:20Z

Can we find a pattern which should be marked as inconclusive?

danmar · 2019-10-04T10:18:23Z

Can we find a pattern which should be marked as inconclusive?

As far as I know the intention of this checker is to warn for that kind of code... so if you don't like it you should suppress it.

As far as I remember we used to suppress these warnings in Cppcheck. Not sure if/when/why that suppression was removed.

danmar · 2019-10-04T10:19:17Z

@tgnottingham can you please add the suppression again.. add it at the top of the file cppcheck/.travis_suppressions.

tgnottingham · 2019-10-05T06:59:40Z

@danmar Done. Also fixed the issue in lib/valueflow.cpp. Thanks all for the feedback and discussion.

pfultz2 · 2019-10-05T09:07:19Z

IMO & is used by intention here since diag (when not specified otherwise) does insert the token. So the function has side effects and with && the second condition would maybe not tested and thus the token not inserted.

We should check if the LHS or RHS has side effects. We do something similar for isSameExpression. I believe you can use the isConstExpression function in astutils.h to check for this case.

pfultz2 · 2019-10-05T09:21:13Z

The code referenced in the following warnings looks like it was written using bitwise operations to be shorter

The intent is to use && without short-circuiting, which is why & is used.

If bitwise operations should be used, the variable types could be changed to integers.

But that would imply the functions returns something other than true or false which is not the case. And if we were to return true with an int, should we return 1 or 2 or 3? Changing to int can lead to other types of code smell.

danmar · 2019-10-05T09:19:33Z

                    ValueFlow::Value result(0);
                    result.condition = value1.condition ? value1.condition : value2.condition;
-                    result.setInconclusive(value1.isInconclusive() | value2.isInconclusive());
+                    result.setInconclusive(value1.isInconclusive() || value2.isInconclusive());


Assuming that Value::isInconclusive() is inlined, I guess this will just cause extra CPU load. Instead of A|B there will be A ? 1 : B .. so in best case you replaced a read with a jump (which is probably slower as far as I know).. and in worst case this just means an extra jump.

I think for readability reasons we should allow bitwise or.

Assuming that Value::isInconclusive() is inlined, I guess this will just cause extra CPU load. Instead of A|B there will be A ? 1 : B .. so in best case you replaced a read with a jump (which is probably slower as far as I know).. and in worst case this just means an extra jump.

True, though the same could be said of many uses of boolean operators where short-circuiting isn't being utilized. I suspect the general practice is to use boolean operators by default, and use bitwise operators in select optimizations.

True, though the same could be said of many uses of boolean operators where short-circuiting isn't being utilized. I suspect the general practice is to use boolean operators by default, and use bitwise operators in select optimizations.

Yes. The checker works as it should if it warns for this code. I just don't want that we use this checker in our own code.

I like this PR :-)

I feel I can merge this PR if this change in ValueFlow is reverted.

Thanks. :) Reverted the change to ValueFlow.

danmar · 2019-10-05T09:21:07Z

 shadowFunction
 functionConst
 functionStatic
+bitwiseOnBoolean:lib/checkcondition.cpp


I suggest that you suppress this in all files.

Instead of suppressing this, can we not warn when isConstExpression(tok->astOperand1()) is false?

Updated to suppress in all files.

We should check if the LHS or RHS has side effects.

My concern is that this would skip cases where this check is most useful. If the RHS has side effects and the bitwise op is unintentional, then the check as-is reveals a potential bug. That may be fairly rare, but it's hard to be sure of the programmer's intent.

We should check if the LHS or RHS has side effects.
Instead of suppressing this, can we not warn when isConstExpression(tok->astOperand1()) is false?

I did not want to have this checker personally.. so I am not 100% sure. But I think those that wanted to have this checker wanted it to warn always for boolean operators.

I think we should keep these warnings.. but we can use a different ID if we see some special case.. however I am thinking that "separation" can be done separately. This PR is about making the checker work as it was intended to start with as far as I see.

If the logic behavior might be changed if | is replaced by || I think it would be best to mention this in the warning message.. amd possible use a different ID.. but I think that can be done in a separate PR also.

Use AST-based tests in favor of token-based tests for greater coverage.

* Fix false negatives in checkBitwiseOnBoolean Use AST-based tests in favor of token-based tests for greater coverage. * Travis: add suppressions for bitwiseOnBool

danmar reviewed Oct 5, 2019

View reviewed changes

pfultz2 reviewed Oct 5, 2019

View reviewed changes

Comment thread lib/checkbool.cpp Outdated

tgnottingham added 2 commits October 5, 2019 13:20

Fix false negatives in checkBitwiseOnBoolean

c1bd259

Use AST-based tests in favor of token-based tests for greater coverage.

Travis: add suppressions for bitwiseOnBool

80732fe

danmar merged commit 0950a97 into cppcheck-opensource:master Oct 6, 2019

Conversation

tgnottingham commented Sep 26, 2019

Uh oh!

danmar commented Sep 26, 2019

Uh oh!

amai2012 commented Sep 26, 2019

Uh oh!

tgnottingham commented Sep 27, 2019

Uh oh!

amai2012 commented Sep 27, 2019

Uh oh!

danmar commented Sep 27, 2019

Uh oh!

amai2012 commented Oct 4, 2019

Uh oh!

versat commented Oct 4, 2019

Uh oh!

amai2012 commented Oct 4, 2019

Uh oh!

danmar commented Oct 4, 2019

Uh oh!

danmar commented Oct 4, 2019

Uh oh!

tgnottingham commented Oct 5, 2019

Uh oh!

pfultz2 commented Oct 5, 2019

Uh oh!

pfultz2 commented Oct 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants