Re: [PATCH 0/7] static analysis fixes for skalibs
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
Re: [PATCH 0/7] static analysis fixes for skalibs
В письме от 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. smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH 0/7] static analysis fixes for skalibs
On 13/03/2015 16:47, Roman Khimov wrote: 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. Fine, I removed it. *shrug* 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. Yes, some of the code in src/sysdeps/ is not supposed to be run, but only compiled and/or linked. It's there to test for feature of the host system. It's purely stylistic thing, so if you as a git master owner think it doesn't make sense, I'm fine with it. Oh, it makes sense, but I don't like this approach. It smells too much of defensive programming, in which you do things just to be sure. Well, when in doubt, add parentheses is the wrong approach to me, the right approach being when in doubt, RTFM and remove the doubt. 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. Oh, OK. I understand now. And you're right, n isn't modified in child_spawn(). Fixes committed, new release ready. Thanks again! -- Laurent
