Re: [PATCH 0/7] static analysis fixes for skalibs

From: Roman Khimov <khimov_at_altell.ru>
Date: Fri, 13 Mar 2015 18:47:18 +0300

В письме от 13 марта 2015 16:16:26 пользователь Laurent Bercot написал:
> 1/7: I incremented 's' for clarity, because that's I always do in scanning
> functions. Normally the compiler ignores the useless increments and this
> does not worsen the resulting code.
> Do you think the increment actually takes away from clarity ? Or does
> clang emit a warning about it ? (gcc does not.) I think it's harmless, but
> if you disagree, I don't really care either way.

Both scan-build and cppcheck complain here. Sure, it's not an error, just a
harmless dead code, but well, tools don't like dead code and I personally
don't like it either, so IMO it's better to drop it if there are no valid
reasons for it to stay.

Speaking of dead code, cppcheck also sees some in src/sysdeps/trycmsgcloexec.c
and src/sysdeps/trygetpeerucred.c, but from what I see those are currently
stubs, so I didn't touch them.

> 4/7: I've actually tried going the opposite way lately: reducing the
> amount of parentheses I'm using. I think it's better to ensure your C
> programmers know their operators' precedence than to defensively add
> parentheses whenever there's uncertainty. Uncertainty is a bad thing - if
> you're not sure, read the language spec. Besides, usually, the language's
> precedence makes sense. So I'm not going to apply that one;

It's purely stylistic thing, so if you as a git master owner think it doesn't
make sense, I'm fine with it.

> is there a way
> to silence that type of report in the static analyzer ?

Sure, it can be suppressed in several ways, from cppcheck's command line
parameters to specially-crafted code comment.

> 6/7: I'm surprised your tools detected that one, but not the zillion other
> cases in skalibs. There are lots of functions that do not modify their
> arguments but do not declare them as const... I basically never bother
> using const qualifiers in function arguments - force of habit; and I think
> compilers are able to themselves deduce the const status of those
> arguments, so the code isn't worse for it. At some point, when OCD
> overcomes laziness, I may make a complete pass over all of my code and fix
> that, but I don't think it's needed, and changing it in one function only
> doesn't really make sense.

Well, this one is from me personally, fixing 5/7 and 7/7 I wasn't sure that
nothing changes 'n' because child_spawn() is not a 10-line function and 'n' is
not fun to search for. Making it const easily ensures that 'n' is the same 'n'
in error handler as it was in the beginning of the function.

None of the tools actually complain about const/non-const here and yes, there
are tons of similar possible cases.




Received on Fri Mar 13 2015 - 15:47:18 UTC

This archive was generated by hypermail 2.3.0 : Sun May 09 2021 - 19:38:49 UTC