Use valueflow in unsigned less than zero checker

[rikardfalkeborn]

The unsigned less than zero checker looked for patterns like "<= 0".
Switching to use valueflow improves the checker in a few aspects.

First, it removes false positives where instead of 0, the code is using
0L, 0U, etc. Instead of having to hard code the different variants of 0,
valueflow handles this automatically. This fixes FPs on the form

uint32_t value = 0xFUL;
void f() {
	if (value < 0u)
	{
		value = 0u;
	}
}

where 0u was previously not regocnized by the checker. This fixes #8836.

Morover, it makes it possible to handle templates properly. In commit
fa07659, all warnings inside templates
were made inconclusive, since the checker had no idea if the "0" came
from a template parameter or not.

This makes it possible to not warn for the following case which was
reported as a FP in #3233

template<int n> void foo(unsigned int x) {
if (x <= n);
}
foo<0>();

but give a warning for the following case

template<int n> void foo(unsigned int x) {
if (x <= 0);
}

Previously, both these cases gave inconclusive warnings.

Finally, it makes it possible to give warnings for the following code:

void f(unsigned x) {
	int y = 0;
	if (x <= y) {}
}

[danmar]

I don't know why we have written only "check(" on the first line. I like condensed test cases so I can get as much overview as possible when looking at the test code. I don't want that we change the pattern now in this pull request. But if you later want to do that, feel free to convert all the test cases in this file.

[rikardfalkeborn]

Agreed, I just tried sticking with the style in the file. I can take a look later. Would it make sense to shorten the test cases further so that

bool f(unsigned x) {
    if (x < 0)
        return true;
    return false;
}

is replaced with

void f(unsigned x) {
    if (x < 0) {}
}

(there are quite a few test cases like that and the return statements don't really add anything to the test case)

[danmar]

Agreed, I just tried sticking with the style in the file.

Yes, that is better than mixing different styles.

Would it make sense to shorten the test cases further so that

Yes that would be great.

[danmar]

👍 for using ValueFlow.

[danmar]

I guess there are situations where it will not be obvious that RHS is 0 when looking at the code and then the warning should explain why Cppcheck thinks so. I suggest that you use Check::getErrorPath somehow when reporting this warning.

I would say that when the value is a integer literal then it is preferable to not use Check::getErrorPath. Then it is obvious that it's 0

[danmar]

We should have some utility function somewhere that can determine if an expression is a "literal". However I do not find it. The isConstExpr sounds good but does not do it. The isConstVarExpr is pretty close, it's just a shame that it returns true for constant variables. I will try to find something...

[danmar]

I would say that when the value is a integer literal then it is preferable to not use Check::getErrorPath.

I am not sure but maybe you can ignore this advice. Does it generate good output if you always use Check::getErrorPath?

[rikardfalkeborn]

#define z 0
void f(unsigned x) {
    if (x < 0) {}
}
void g(unsigned x) {
    int y = 0;
    if (x < y) {}
}

void h(unsigned x) {
    if (x < z) {}
}

With Check::getErrorPath:

cppcheck --enable=style unsigned-example.c
Checking unsigned-example.c ...
[unsigned-example.c:3]: (style) Checking if unsigned expression 'x' is less than zero.
[unsigned-example.c:7]: (style) Checking if unsigned expression 'x' is less than zero.
[unsigned-example.c:11]: (style) Checking if unsigned expression 'x' is less than zero.

cppcheck --enable=style --verbose unsigned-example.c
Checking unsigned-example.c ...
Defines:
Undefines:
Includes:
Platform:Native
[unsigned-example.c:3]: (style) The unsigned expression 'x' will never be negative so it is either pointless or an error to check if it is.
[unsigned-example.c:6] -> [unsigned-example.c:7]: (style) The unsigned expression 'x' will never be negative so it is either pointless or an error to check if it is.
[unsigned-example.c:11]: (style) The unsigned expression 'x' will never be negative so it is either pointless or an error to check if it is.

So yes, the output if there is a "literal" seems to fine. However, in order to get the full "path" in other cases, one has to specify --verbose (see below). Would that be sufficient?

The condition for getting the full path is (verbose I understood what it does, I didn't check the other two):

    (mSettings->verbose || mSettings->xml || !mSettings->templateLocation.empty())

[rikardfalkeborn]

mSettings->xml is xml output I suppose. Then I think this should be ok behaviour (it's the same way the zerodiv-checker works).

Btw, do you prefer me to squash the changes before I push or do you prefer 'delta-commits' (for simpler reviewing?).

[danmar]

With Check::getErrorPath:

that seems to work well. I think the --template=gcc output is much more useful for multilocation messages.

Btw, do you prefer me to squash the changes before I push or do you prefer 'delta-commits' (for simpler reviewing?).

Feel free to squash it.

[rikardfalkeborn]

New version uses getErrorPath(). I also fixed a bug in the unsigned larger than zero checker that tried printing the variable name. However, it did so by printing the astOperand-token, which didn't work well if there was something like x - y >= 0' since the astOperand was the minus sign. Using expressionString()` like in the check for unsigned less than zero cured that. (one could argue that this should have been a separate PR).