On 13/03/2015 15:24, Roman I Khimov wrote:
> Hello.
>
> Here at Altell we daily pass all of our project's software (and that is kinda
> whole distribution) through special 'static analysis' build that doesn't
> actually produce any output other than reports from two (currently) tools:
> cppcheck and Clang's scan-build.
>
> As we've added skalibs into our project we've immediately received reports for
> skalibs. It's nice overall, but there are some things that can fixed or
> improved, thus this patch series for your review and (probably) merge.
Hi Roman,
Thanks a lot for the reports! This is very interesting. I'll try and see if
I can use cppcheck and scan-build myself in the future, if it can detect
errors like the ones you submitted.
My comments on your patches:
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.
2/7: applied.
3/7: applied.
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; is there a way to silence that type of
report in the static analyzer ?
5/7: applied.
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.
7/7: applied.
I'll commit when I've made sense of the mininetstring_write thing. ;)
Thanks again!
--
Laurent
Received on Fri Mar 13 2015 - 15:16:26 UTC