When an analyzer or compiler issues a warning, sometimes it's hard to identify whether it's a false positive or a real error. Analyzers/compilers can be technically right, but the code also works correctly. What to do? Perhaps that's a reason to make the code more concise.
All static code analyzers as well as compliers have a well-known shortcoming — false positives. You can suppress obvious false positives in many ways. There are actually scenarios explaining how to start using static analysis safely in large legacy projects. These projects usually contain a lot of false positives.
Interesting that sometimes it's hard to say if warnings are false or not. That's what we're going to discuss today.
Sometimes the analyzer/compiler is absolutely right when issuing a warning. However, the code works exactly as intended. There is no error in it. This usually indicates that the code is redundant, overcomplicated, or "smelly". Well, let's make it clear and jump right in a practical example of the code fragment from Blender project:
static bool lineart_do_closest_segment(....)
{
int side = 0;
....
/* No need to cut in the middle,
because one segment completely overlaps the other. */
if (side) {
if (side > 0) {
*is_side_2r = true;
*use_new_ref = true;
}
else if (side < 0) { // <=
*is_side_2r = false;
*use_new_ref = false;
}
return false;
}
....
}Here the PVS-Studio analyzer issues a warning "V547: Expression 'side < 0' is always true" on the line marked with a comment.
Let's delete all unnecessary things and consider the code in more detail.
if (side) {
if (side > 0) {
*is_side_2r = true;
*use_new_ref = true;
}
else if (side < 0) {
*is_side_2r = false;
*use_new_ref = false;
}
return false;
}The first condition cuts off the cases where the side variable is equal to 0. Then, if the side variable is less or greater than zero, the is_side_2r and use_new_ref variables are assigned to different values. And the function terminates.
When the side < 0 condition is visited, the analyzer considers that the variable is always less than 0. That's why, it issues a warning.
The analyzer is technically right. True/false conditions often indicate a bug or other code error. There are hundreds of error examples that the V547 diagnostic identifies.
However, there is no error here. The code is a little redundant. The developer obviously prefers eye-pleasing coding or using defensive programming style. It may also be possible that the code became redundant during the refactoring process. This also happens — I described some of such cases in my previous articles.
Nevertheless, let's get back to the analyzer's warning. The developer is right. And the analyzer is right. What to do? The easiest way is to suppress a particular warning with a special comment.
if (side) {
if (side > 0) {
*is_side_2r = true;
*use_new_ref = true;
}
else if (side < 0) { //-V547
*is_side_2r = false;
*use_new_ref = false;
}
return false;
}This way is not my favorite one. Let's discuss other possible ways for changing the code. It would be great to remain code as obvious and beautiful as it was. Actually, the original code was quite good and readable.
And let me note that I won't give you perfect solution. Further on, we'll discuss several ways to refactor this code. So that everyone can choose a way that they like more or that fits for a code style (adopted by the team).
The next simple way to remove the analyzer's warning is to delete the unnecessary check.
if (side) {
if (side > 0) {
*is_side_2r = true;
*use_new_ref = true;
}
else {
*is_side_2r = false;
*use_new_ref = false;
}
return false;
}Actually, everything's the same. We deleted one condition, and the warning disappeared. But I think the code became less readable. Using such a way, we have to remember where the side variable has what value.
If I were writing the code, I would do as follows:
if (side > 0) {
*is_side_2r = true;
*use_new_ref = true;
return false;
}
else if (side < 0) {
*is_side_2r = false;
*use_new_ref = false;
return false;
}There are no nested if statements. The code became less complicated. It is more readable and obviously clear. That's probably the way I would have settled on.
But if you are a fan of short coding, you would definitely like the following way. What would you think about this?
if (side) {
const bool sideGreaterThanZero = side > 0;
*is_side_2r = sideGreaterThanZero;
*use_new_ref = sideGreaterThanZero;
return false;
}This code is short and obvious. But in my opinion, it is less readable than the previous fragment. Well, maybe this is a matter of taste.
Could it be even shorter? Oh, yes:
if (side) {
*use_new_ref = *is_side_2r = side > 0;
return false;
}To be honest, I'm not so thrilled about this code. It seems like we want to boast about the way we shorten the code and say: "Look what I can do". It's not a good idea to use this way. The article turned out really well, though. It would be great to pay attention to the redundant condition and perform refactoring. As a result, we can reduce the code lines from 11 to 4.
It's up to you to decide which code change you want to make. My mission is to demonstrate the possible ways to discover the nature of false positives in an analyzer/compiler. So, there is no need for rush to suppress the warning. This is probably a good reason to refactor the code a little and simplify it.
Additional links: