stumbling upon a non-trivial bool scenario in C++

Question

When Cppcheck runs over this code,^[1] it complains about an error:

void bool_express(bool aPre, bool aX, bool aPost)
{
    bool x;
    const bool pre = aPre;
    if (pre) {
        x = aX;
    }
    const bool post = aPost;

    // error checking passage of the original code:
    if ( !pre || !x || !post ) {
        if ( !pre ) {
            trace("pre failed");
        } else if ( !x ) {       // <-- HERE Cppcheck complains
            trace("x failed");
        } else {
            trace("post failed");
        }
    } else {
        // success passage of the original code:
        trace("ok");
    }
}

This is the message, that makes me nervous:

Id: uninitvar
Summary: Uninitialized variable: x
Message: Uninitialized variable: x

I think it's a false positive, but I admit that this may not be obvious. Nevertheless I don't want to touch that code, because it's old and a lot heavier than this extracted version.

Have you ever experienced situations like this? How to deal with it?

^[1] This code is reduced to its bones, the original code establishes a precondition (pre), does something (x), then forces a postcondition (post), after that, some error conditions are checked. I transformed the runtime-results of the functions that are called and then stored in pre, x, and post into the 3 arguments of my test case.

We're using Klockworks for static code analysis, and it shows the flow in which `x` is used uninitialized, if Cppcheck has this option post it and maybe they see something everyone else is missing, although it looks like a false positive to me — ZivS, Aug 08 '16 at 13:14
Did you make sure the reduced version still produces the CppCheck warning? — nwp, Aug 08 '16 at 13:39
How about you just initialize `x` to false? I cannot imagine a realistic scenario in which that would blow up. — nwp, Aug 08 '16 at 13:47
@nwp Finally you are right ;) As to check if the bug has been fixed in Cppcheck, I have (after preparing the question) a test project, so I will probably better initialize may original "x" and schedule a meta check for Cppcheck 1.76. — Wolf, Aug 08 '16 at 14:34
@nwp concerning *`Did you make sure the reduced version still produces the CppCheck warning`*, yes, I did. — Wolf, Aug 08 '16 at 14:36

KidneyChris · Answer 1 · 2016-08-08T15:15:40.397

1

The static analysis appears to be complaining because if pre is false, then x is never set.

Your code is structured such that the value of x is never accessed if pre is false - I'd argue that the static analyser isn't giving a useful output in this case.

Enumerating the various cases we have (so we can be reasonably sure that it's cppcheck and not us!):

The first statement in which x is accessed is in the line if ( !pre || !x || !post ) - due to short-circuiting evaluation: if( A || B || C ) doesn't evaluate B or C if A is true; hence we never try to read an uninitialised x (since x is only uninitialised if pre is false, in which case we stopped evaluated the expression!)
The second usage is in

if ( !pre ) { trace("pre failed"); } else if ( !x ) { // <-- HERE Cppcheck complains

Again, we can only hit the offending line if pre was true (in which case x is properly initialised).

From this, we can conclude that either:

The actual code mistakenly tries to read x in some condition even is pre is false, and you've missed it when building the example (sometimes the logical flow of a program can be a bit obtuse)
The static analyser is lazy and spots the line else if( !x ) and can't determine if this line is reachable with an uninitialised value.

From the code you've provided, you shouldn't be concerned: the static analysis tool is technically correct that x can be uninitialised, but in those cases it's not used (and hence probably shouldn't be warning you).

I'd recommend assigning x a default value if you're not confident, or if the actual logic is exceedingly obtuse.

edited Aug 08 '16 at 15:15

answered Aug 08 '16 at 13:52

KidneyChris

857
5
12

Better use the C/C++ version of **or** (`||`) instead of the **binary or** operator (`|`). – Wolf Aug 08 '16 at 14:05
@Wolf Whoops! Corrected! – KidneyChris Aug 08 '16 at 14:24
Please **don't refer to short-circuiting here, because it's simply wrong**. Short-circuiting is used for boolean *expressions* and in the case shown, the only expression that is (most probably) short-circuit evaluated is `( !pre || !x || !post )`, and it is passed in 7 of 8 cases, especially in all cases where `!pre`, the point is that `x` is only evaluated if `!!pre`, in other words if `pre` is `true`. – Wolf Aug 08 '16 at 14:26
Please have a look on the second issue as well. Thanks :) – Wolf Aug 08 '16 at 14:28
@Wolf There's two uses of `x` in your example, one is `( !pre || !x || !post )`, the other is in the `else if` block. In the former, `!x` isn't evaluated if `!pre` (and if `!pre`, then `x` is undefined) due to short-circuiting. I agree that it isn't the "whole story" (notable `else if(!x)` which is obviously not evaluated if `!pre`), but I disagree that it's wrong outright. Unless I missed something? – KidneyChris Aug 08 '16 at 14:32
@Wolf Edited to clarify that not every usage of `!x` is skipped due to short-circuiting. (I'm also aware your answer has pegged this as an issue with cppcheck in unreachable code; it's still an interesting question) – KidneyChris Aug 08 '16 at 14:37
Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/120436/discussion-between-wolf-and-kidneychris). – Wolf Aug 08 '16 at 14:39
Following an extended chat discussion, I've edited the answer to hopefully be a bit more well-rounded, and not focussed so much on a singular issue (which wasn't all that important to the problem anyway). – KidneyChris Aug 08 '16 at 15:27

Wolf · Answer 2 · 2016-08-08T14:09:54.977

It is a false positive in Cppcheck. I solved it by adding an inline suppression:^[1]

    if ( !pre ) {
        trace("pre failed");
    // cppcheck-suppress uninitvar
    } else if ( !x ) {
        trace("x failed");
    } else {
        trace("post failed");
    }

and I also brought it to the attention of the Cppcheck developers:

#7663 (False positive: uninitialised variable in unreachable code)

^[1] I decided not to initialize the variable. This is not for performance reasons, but to be informed about the bug being fixed in a future release, when Cppcheck will say

Id: unmatchedSuppression
Summary: Unmatched suppression: uninitvar
Message: Unmatched suppression: uninitvar

score 0 · Answer 3 · answered Aug 08 '16 at 13:42

0

If your pre condition is false than x will be uninitialized. At line if ( !x ) CppCheck warns about usage of indeterminate value. To fix it initialize x variable.

answered Aug 08 '16 at 13:42

Nikita

6,270
2
24
37

If `pre` is `false` then `!x` is not checked due to one of short circuiting or lazy evaluation, can't remember what it is called. – nwp Aug 08 '16 at 13:44
I think it's not even short-circuiting, but just impossible that `x` is uninitialized if `pre` is true. Well, initializing `x` will not harm, yes. – Wolf Aug 08 '16 at 13:50
@nwp @Wolf You right, `!x` doesn't checked in `!pre || !x || !post` in this case. But seems CppCheck doesn't have proper value propagation to understand that `if ( !x )` is also not executed. Also it's better to warn about a possible problem here, because it can cause a real problem when outer condition `!pre || !x || !post` will be changed. – Nikita Aug 08 '16 at 13:51

stumbling upon a non-trivial bool scenario in C++

3 Answers3

Linked