unused-value-checker (not) for init-value-overwrite-cases

[Martin]

Hi,

in our code there are many findings like
"Variable 'foo' is assigned a value that is never used."
which result from variables being initialized and overwritten later (without having been read before).

I'd suggest for the unused-value-checker to skip cases where the overwritten value was one from variable initialization. I'd argue:
- I'd rather have an overwritten, unused value than an uninitialized variable; so better initialize each variable, even if the value is overwritten later.
- There's another cppcheck checker for "redundant initialization", which should be cover the cases mentioned above.

What do you think?

Best regards,
Martin

[Daniel Marjamäki]

please provide some example that we can discuss. The idea is to have a separate id for redundant initialization; redundantInitialization.
You should be able to suppress that and still get the other warnings that you want.

[Martin]

For this code

void foo()
{
   uint16_t tempNodeAdress = 0x0000;
   tempNodeAdress = 0x0001;
}

cppcheck says:
Variable 'tempNodeAdress' is assigned a value that is never used. [unreadVariable]

I would like know a way to seperate these kinds of findings of this checker from the ones where variable initialization isn't involved, like:

void foo()
{
   uint16_t tempNodeAdress = 0x0000;
   tempNodeAdress = 0x0001; //ok
   tempNodeAdress = 0x0002; //not ok
}

Here cppcheck says:
/home/user/Desktop/21-06-01.cpp:5:19: style: Variable 'tempNodeAdress' is reassigned a value before the old one has been used. [redundantAssignment]
tempNodeAdress = 0x0002;
^
/home/user/Desktop/21-06-01.cpp:4:19: note: tempNodeAdress is assigned
tempNodeAdress = 0x0001;
^
/home/user/Desktop/21-06-01.cpp:5:19: note: tempNodeAdress is overwritten
tempNodeAdress = 0x0002;
^
/home/user/Desktop/21-06-01.cpp:5:19: style: Variable 'tempNodeAdress' is assigned a value that is never used. [unreadVariable]
tempNodeAdress = 0x0002;

[Daniel Marjamäki]

For this code

In that code, I would suggest that you remove the variable. Maybe simplified a bit too far..

[Daniel Marjamäki]

I would guess that the variable is used somehow but the usage is hidden for Cppcheck? For example you might have a #if? I have seen cases where it is possible to move variables into the #if scope or refactor the code and get cleaner code. Even if Cppcheck warning was unfortunate it did point out a code smell.

[Martin]

Mh ... I could have kept some of the other code I deleted to "bloat up" the example, but the above minimalized snippet shows exactly what I mean. I analyzed a file containing only this code, no #if and so on. Are you unable to reproduce? (--enable=all)

Maybe to clarify: I'm looking for a way to only get a warning for the 0x0002 line, and not for the 0x0001 line. As I explained, I think these findings differ in criticality.

[Daniel Marjamäki]

If the variable is never read anywhere then I see no reason to keep the variable. No matter if you initialize or assign. But you indicated that the variable is used somehow earlier, because you wanted to avoid uninitialized variable usage.

[Martin]

And isn't the code also an example of "redundant initialization"? Why is there no such finding? I think I may just be confused about the respective checkers and their scope: unreadVariable, redundantAssignment, redundantInit, ...

[Daniel Marjamäki]

yes. I was a bit surprised also. I have not worked on this much lately so don't know the details .. maybe there is a bailout here , I am not sure maybe initialization with 0 is treated differently.

[Daniel Marjamäki]

the idea is ;
* unreadVariable - the variable is written somewhere but never read anywhere - it should be possible to remove the variable
* redundantAssignment - the variable is assigned a value that is never used
* redundantInitialization - the variable is initialized a value that is never used

[Martin]

Ok, maybe I'll try with another example:

void MapErrorEval::FreeLastArrayIdx()
{
   int32_t   i = 0;
   if(m_Idx == HistSize)  // Free last index data in the array and store new values in it.
   {
      --m_Idx;

      for ( i = 0; i < HistSize - 1; i++ )
      {
         m_Hdg_hist[i]  =  m_Hdg_hist[i+1];
      }

      m_Hdg_hist[i].headingfromFL = 0;
      m_Hdg_hist[i].headingFromSeg = 0;
   }
}

Here cppcheck says:
/home/user/Desktop/21-06-01.cpp:3:14: style: The scope of the variable 'i' can be reduced. [variableScope]
int32_t i = 0;
^
/home/user/Desktop/21-06-01.cpp:3:16: style: Variable 'i' is assigned a value that is never used. [unreadVariable]
int32_t i = 0;
^

I think it would make more sense (and it would be more consistent with your explanation) here to output a redundantInitialization error, and not this "unused value" error as shown. Then I could deactivate the redundantInitialization checker, and focus on the "unused value" findings which are (in my opinion) more critical (-> the ones were the overwritten value does not come from initialization).

[Martin]

Just pinging here to find out if this is not a topic where you think some change is reasonable, or if I should make another attempt to explain, or if this is postponed until cppcheck2.5 is released, or ...

[Daniel Marjamäki]

yes I agree. In that example code it would be better to write redundantInitialization. I have created https://trac.cppcheck.net/ticket/10324

[fenugrec]

As Daniel stated in a comment on #10324, "people often use "redundant initialization" by design." Agreed; still getting false positives on cppcheck 2.10 in certain cases :

    HANDLE hCom_t = NULL;

    ....
    cv = someAPI();
    if (cv) {
        hCom_t = CreateFileA( ....);
        if (hCom_t != INVALID_HANDLE_VALUE) {
            ....
**************
 style: Variable 'hCom_t' is assigned a value that is never used. [unreadVariable]
 HANDLE hCom_t = NULL;
               ^

Not sure why it's not triggering a redundant-initialization , maybe because of the conditional block... In any case I'll have to use a local suppression for the moment, I don't want to disable this check for the whole codebase.

[CHR]

Can you provide a minimal self-contained example?

[fenugrec]

Of course :

int test(void) {
    long cv;
    HANDLE hCom_t = NULL;

    cv = check();
    if (cv == ERROR_SUCCESS) {
        hCom_t = CreateFileA();
        (void) hCom_t;
    }
    return 0;
}

*************

 $ cppcheck --enable=style cppcheck_10324.c
 cppcheck_10324.c:3:16: style: Variable 'hCom_t' is assigned a value that is never used. 
[unreadVariable]
 HANDLE hCom_t = NULL;
               ^

[CHR]

Thanks, I can reproduce. Have you tried passing --library=windows? Otherwise HANDLE is an unknown type.

[fenugrec]

My build system has "--platform=win32A" and some other related flags, yes. You could replace HANDLE with int without changing behaviour for this test.

[CHR]

I meant to say that I don't get the warning with --library=windows.
This seems to be related: https://trac.cppcheck.net/ticket/10324

[fenugrec]

I meant to say that I don't get the warning with --library=windows.

Oh interesting. I still get the warning with that flag here... cppcheck 2.10 on archlinux