hi, i agree that we should focus on code stability :)
re: malloc() and 0: linux overcommits, i.e. it will likely never return 0 even if your memory is all full. this means checking for 0 is completely useless in this context. roman setup coverity scan for rawspeed in the past. i thought it was also for darktable but don't remember. cheers, jo On Sun, Jan 27, 2019 at 3:39 PM Mark Feit <mf...@notonthe.net> wrote: > > On 1/27/19 6:18 AM, Heiko Bauke wrote: > > > > Currently, there is an offer for open source developers to get a free > > license for the PVS-Studio Analyzer tool. I got one and applied the > > tool to the darktable master branch. > > ... > > I was not yet able to study the results in detail. There might be a > > lot of false positives or just minor issues. But I expect to find > > also more serious things. > > I did a survey of the report index, dove into a few dozen of the errors > and found it to be a high-quality report with little in the way of false > positives. > > A large percentage of the warnings are related to assumptions that > pointers will not be NULL and a large percentage of those are directly > or indirectly related to unchecked returns from malloc() and calloc(). > That could be made to go away by writing wrapper functions that check > what's returned and halt the program nicely in the rare event of a > failure. Knowing it was that would send the developers on fewer goose > chases to find the cause of a SIGSEGV further down. If performance is a > concern (not that malloc() is a real screamer to begin with), the > solution can be split in a way that makes it attractive for the > optimizer to inline the check. > > Those aside, the others I looked at seem legitimate and are worth > fixing. None of it will require major work. > > > For example, > > https://rabauke.github.io/darktable_analyze/sources/collection.c_4.html#ln144 > > looks very fishy to me. > > That's definitely code I'd kick back during a review with a > recommendation that it be written into a small function because the same > logic is used repeatedly and that variable assignment inside the > condition is ugly. You might get a pull request for that shortly. ;-) > > > If I can offer a few additional comments: > > Before committing the project to PVS-Studio, it would be worth > evaluating some of the alternatives, especially those that are > open-source. I think it's great that PVS offers a free license for so > many situations, but there is the risk of having to go through and > re-flag all of the spots in the code where warnings were suppressed > should they change the license terms or go out of business and not > release the sources. > > Once the code is to a point where the analyzer has nothing to squawk > about, static analysis needs to be repeated regularly. This could be > done as a simple cron job that notifies the developers when something > crops up or as a check run web hook to prevent code that doesn't pass > from being committed to GitHub. I have a system with cycles and space > to play that role if needed and can make whatever configuration and > scripts I develop part of the dt sources so others can run it. > > Doing the cleanups for this is a great opportunity for someone like me > who wants to give something back and doesn't have the time to take on a > major project. It isn't glamorous work, but I'd be happy to do it. > > --Mark > > > ___________________________________________________________________________ > darktable developer mailing list > to unsubscribe send a mail to darktable-dev+unsubscr...@lists.darktable.org > ___________________________________________________________________________ darktable developer mailing list to unsubscribe send a mail to darktable-dev+unsubscr...@lists.darktable.org
