eteran / c-vector

A dynamic array implementation in C similar to the one found in standard C++
MIT License
737 stars 109 forks source link

cppcheck #30

Closed marekmosna closed 2 years ago

marekmosna commented 2 years ago

Hello @eteran I made static c code analysis by cppcheck and here is the result. If you want to fix it let me know, or close the issue otherwise. I can even integrate it into CI but there is already any CodeQL static analysis. I don't know what it doing but cppcheck I know.

cppcheck --enable=all --inconclusive test.c 
Checking test.c ...
test.c:105:9: error: Invalid memmove() argument nr 3. The value is -4 but the valid values are '0:'. [invalidFunctionArg]
        cvector_insert(b, 2, 4);
        ^
test.c:106:9: error: Invalid memmove() argument nr 3. The value is -4 but the valid values are '0:'. [invalidFunctionArg]
        cvector_insert(b, 2, 2);
        ^
test.c:107:9: error: Invalid memmove() argument nr 3. The value is -8 but the valid values are '0:'. [invalidFunctionArg]
        cvector_insert(b, 3, 3);
        ^
test.c:50:9: style: The if condition is the same as the previous if condition [duplicateCondition]
    if (v) {
        ^
test.c:33:9: note: First condition
    if (v) {
        ^
test.c:50:9: note: Second condition
    if (v) {
        ^
test.c:53:13: portability: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'size_t {aka unsigned long}'. [invalidPrintfArgType_uint]
            printf("v[%lu] = %d\n", i, v[i]);
            ^
test.c:88:13: portability: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'size_t {aka unsigned long}'. [invalidPrintfArgType_uint]
            printf("a[%lu] = %d\n", i, a[i]);
            ^
test.c:114:13: portability: %lu in format string (no. 1) requires 'unsigned long' but the argument type is 'size_t {aka unsigned long}'. [invalidPrintfArgType_uint]
            printf("b[%lu] = %d\n", i, b[i]);
            ^
Checking test.c: NDEBUG...
nofile:0:0: information: Cppcheck cannot find all the include files (use --check-config for details) [missingIncludeSystem]
eteran commented 2 years ago

OK, most of these will be easy. The memmove ones are interesting and may represent actual bugs. So definitely will take a look at those ASAP.

Thanks!

marekmosna commented 2 years ago

Yeah. As soon as you deploy testing framework style and portability issues disappear. The only real issue is that memmove. I know what is going on there and now, I'm trying to find out some fancier solution :1st_place_medal:

eteran commented 2 years ago

Out of curiosity, what version of cppcheck are you using? I ran the same exact command with 2.6.3 (I'm installing 2.7.x now) and it only had style and portability errors listed, the memmove didn't come up.

eteran commented 2 years ago

Hmm, perhaps the memory error check is only available in the "premium" version?

marekmosna commented 2 years ago

Good question. This one is in stable ubuntu:focal tree

$ cppcheck --version
Cppcheck 1.90
eteran commented 2 years ago

Interesting, so maybe they are false positives? For the memmove? Because neither 2.6 or 2.7 emit those (though they do emit a few more style related warnings)

eteran commented 2 years ago

Well I addressed the trivial stuff by using %zu to print size_t types. The only ones left that show up in cppcheck 2.7 are redundant condition warnings, which while true... are also (as far as I can tell) necessary because we're using macros here and they need to work correctly both when used indepdendently and when composed.

marekmosna commented 2 years ago

So I presume that this issue is going to be closed. The higher versions of cppcheck clarify the results. If you don't have any further questions, close it @eteran .

Cheers

marekmosna commented 2 years ago

Maybe at the end, link it to your commit 1b205da9cb26d35c0aa14d8ec02082570fc1be74.

eteran commented 2 years ago

@marekmosna Correct, all findings I was able to both confirm and conclude aren't necessary quirks have been address in commit: https://github.com/eteran/c-vector/commit/1b205da9cb26d35c0aa14d8ec02082570fc1be74

Of course, I will continue to both use and encourage users to search for any memory error class bugs as those are particularly harmful.

Thanks for the help looking into this!