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
