Hi Dilyan, On Sun, 7 May 2017, Дилян Палаузов wrote: > I compiled git/master ... which advances from time to time, so you definitely want to include a more informative data point here, e.g. 4fa66c85f11 (Git 2.13-rc2, 2017-05-04) ... > using the clang 4.0 static analyzer with > > scan-build ./configure --with-libpcre --with-openssl > scan-build make > > and here are the results: > https://mail.aegee.org/dpa/scan-build-git-4fa66c85f11/ > > Please note, that the information is only about what gets actually compiled, > code disabled by #if .. #endif is not considered (e.g. when determining > whether a variable assignment is useless). So you already know that the report is specific to your setup. It may make a lot of sense to actually state what your setup is, i.e. Operating System, installed libraries (and their respective versions), CPU, etc. > There are probably false-positives. Probably. So why don't you give it a try and look through the report? Then summarize your findings here. That would definitely find a warm welcome, I would expect. > However in case of e.g. builtin/notes.c:1018, builtin/reset.c:294 or > fast-import.c:2057 I consider the hints as justified. Okay. And those hint are...? Remember: the easier you make it for your readers to follow your thoughts, the more readers will try to do that. > This is for your information, I wouldn't have a problem if you ignore > the analysis. Thank you for running the scan. However, I would like to encourage you to do better than just call Clang and slap the report verbatim, without much in the way of a manual analysis, onto some website. In fact, I fear that doing it this way is rather counter-productive. Instead, you could try to organize the report a lot better (see the many mails about Coverity in the past): - we already *know* that static analyzers have tons of problems with the way we specify "FLEX_ARRAYS": we define structs to have a 0-item or 1-item array as their last field, then use a variable-length malloc() to tailor the allocated data to the needs of a variable-length array (most often: string). To have these issues in one category would be very helpful, as we could ignore those parts of the report easier. - we also already know quite well that Git's source code often abuses exit() for a garbage collector on short-running processes: In many cmd_*() functions, there are "memory leaks" that are intentional (read: lazy). Again, it would be helpful to be able to fade out those reports. - Likewise, our test helpers abide by a lot less strict rules, as we only use them in the test suite (and therefore it is not *really* necessary to, say, validate the input passed as command-line parameters). Here again, it would be very helpful to put those into a separate category that can be selectively hidden. - there have been a number of patches floating about to fix one or more of the issues reported by Coverity. These patches may not yet have made it into `master`, but at least some of them are in `pu`. A careful analysis which issues reported by Clang would be addressed by `pu` would also be quite welcome. Also: to make it *really* useful for other developers, it would be a good idea for you to try your hand at patching the .travis.yml file in such a way that the static analysis is performed as part of the Continuous Testing, and to contribute said patch. That way, other contributors could not only see how it is done (and copy-paste the commands, including `apt install` calls insofar necessary), but also to see the reports on a trusted website (Travis'). I really would like to believe that you meant to be helpful in sending this mail. But the style does come over as "throwing a couple of scraps to the dogs". I do not believe that is what you wanted, and I think you can turn that impression around by at least some of the things I mentioned above. Ciao, Johannes