> On 25 Feb 2017, at 23:31, Jeff King <peff@xxxxxxxx> wrote: > > On Sat, Feb 25, 2017 at 10:48:52PM +0100, Lars Schneider wrote: > >> >>> On 24 Feb 2017, at 18:29, Samuel Lijin <sxlijin@xxxxxxxxx> wrote: >>> >>> Introduces the scan-build static code analysis tool from the Clang >>> project to all Travis CI builds. Installs clang (since scan-build >>> needs clang as a dependency) to make this possible (on macOS, also >>> updates PATH to allow scan-build to be invoked without referencing the >>> full path). >> >> This is a pretty neat idea. However, I think this should become a >> dedicated job in a TravisCI build (similar to the Documentation job [1]) >> because: >> a) We don't want to build and test a scan-build version of Git (AFAIK >> scan-build kind of proxies the compiler to do its job - I don't if >> this has any side effects) >> b) We don't want to slow down the other builds >> c) It should be enough to run scan-build once on Linux per build > > Yeah. I am all for static analysis, but I agree it should be its own > job. Especially as it can be quite noisy with false positives (and I > really think before any static analysis is useful we need to figure out > a way to suppress the false positives, so that we can see the signal in > the noise). > > Fully a third of the problem cases found are dead assignments or > increments. I looked at a few, and I think the right strategy is to tell > the tool "no really, our code is fine". For instance, it complains > about: > > argc = parse_options(argc, argv, ...); > > when argc is not used again later. Sure, that assignment is doing > nothing. But from a maintainability perspective, I'd much rather have a > dead assignment (that the compiler is free to remove) then for somebody > to later add a loop like: > > for (i = 0; i < argc; i++) > something(argv[i]); > > which will read past the end of the rearranged argv (and probably > _wouldn't_ be caught by static analysis, because the hidden dependency > between argc and argv is buried inside the parse_options() call). > > So there is definitely some bug-fixing to be done, but I think there is > also some work in figuring out how to suppress these useless reports. That makes sense. I suspected that this assignment was intentional but I wasn't sure why. I didn't know about the rearrangement of argv. Apparently an "(void)argc;" silences this warning. Would that be too ugly to bear? :-) > Turning off the dead-assignment checker is one option, but I actually > think it _could_ produce useful results. It just isn't in these cases. > So I'd much rather if we can somehow suppress the specific callsites. > >> I ran scan-build on the current master and it detected 72 potential bugs [2]. >> I looked through a few of them and they seem to be legitimate. If the list agrees >> that running scan-build is a useful thing and that these problems should be fixed >> then we could: >> >> (1) Add scan-build check to Travis CI but only print errors as warning >> (2) Fix the 72 existing bugs over time >> (3) Turn scan-build warnings into errors > > If they are warnings socked away in a Travis CI job that nobody looks > out, then I doubt anybody is going to bother fixing them. > > Not that step (1) hurts necessarily, but I don't think it's really doing > anything until step (2) is finished. Agreed. - Lars