Fix #15031 (False positive: knownConditionTrueFalse reported when calling a function with a known result)

[danmar]

No description provided.

[danmar]

@chrchr-github you added this testcase (c4f754e). therefore I wonder if you can review my fix.
I can understand that we warn here in your test but not on all boolean assignments with known result from some function call. In your test the function call is used in a condition..

[chrchr-github]

I think we can preserve a useful warning by checking for the symbolic value here (warn if a known function result was assigned):
https://github.com/cppchecksolutions/cppcheck/blob/cc71140c09cc842cf6d4c917d9b7fa32ee82bc09/lib/checkcondition.cpp#L1574

[danmar]

hmm..

I believe the original motivation for knownConditionTrueFalse was to flag that there is some dead code path so it would be possible to just remove certain code. If the condition was always true we could remove the condition. If the condition was always false we could remove the whole conditional body.

then we wanted to write warnings for this: if (foo()) flag |= foo(); .. the condition itself might be true or false but it's redundant. the code can be written as flag |= foo();.

I have the feeling that this warning has a different purpose. we would not recommend to remove the condition or the assignment.. it makes the code less explicit if we replace s.g() with true in the assignment..

[danmar]

I don't want to do anything drastic now.. but it feels to me like we should split out some checking after the release.

In your example code.. what is the motivation to warn about the assignment and what is the fix supposed to be?

bool f(S s) {
    if (s.g()) {
        bool b = s.g();
        return b;
    }
    return false;
}

We don't check how the assigned variable is used after the assignment right?

[chrchr-github]

I don't want to do anything drastic now.. but it feels to me like we should split out some checking after the release.

In your example code.. what is the motivation to warn about the assignment and what is the fix supposed to be?

bool f(S s) {
    if (s.g()) {
        bool b = s.g();
        return b;
    }
    return false;
}

We don't check how the assigned variable is used after the assignment right?

The motivation is in https://trac.cppcheck.net/ticket/14392
My idea above would move the warning from the assignment to the return statement.

[danmar]

My idea above would move the warning from the assignment to the return statement.

ok I am still not sure what is the motivation to warn about the return statement? It's not bad to return true from a function. It might be more explicit to write s.g() in the code than true to indicate what the returned value comes from?

I would say that my motivation and description in the doc I wrote does not mean that there should be a warning for the return statement and somehow we should write a motivation and description that match the return statement warnings.

[danmar]

currently the checker does not care about what happens after the assignment. We get a warning here too:

struct S {
        bool g() const { return m; }
        bool m{};
    };

bool f(S s) {
    if (s.g()) {
        bool b = s.g(); // <- Assigned value 's.g()' is always true [knownConditionTrueFalse]
        foo(s,&b);
    }
    return false;
}

I assume your suggestion is that we do not warn here?