On Tue, 2015-01-13 at 08:10 -0500, Matthias Clasen wrote:
On Mon, Jan 12, 2015 at 1:30 PM, Philip Withnall <philip tecnocode co uk> wrote:On Sun, 2015-01-11 at 16:43 +0000, Emmanuele Bassi wrote:hi; thanks for your patches. GTK+ does not use the mailing list to track bugs and patches; could you please open bugs on Bugzilla, instead? just use: https://bugzilla.gnome.org/enter_bug.cgi?product=gtk%2B this will make it easier for us to track the issue, review the code, and commit the change.This looks similar to (but not the same as) the patches I posted from analysing GTK+ with Clang static analyser in November, which are still awaiting review. I really hope we aren't going to end up with duplicate bug reports and duplicate patches because things are not getting reviewed. :'-( https://bugzilla.gnome.org/show_bug.cgi?id=712760I can't say that I agree with them being very similar. Most of Maks' patches were small, to the point, obvious oversight fixes that could be reviewed in a minute, each. The static analysis 'fixes' are an endless stream of mind-numbing can-never-happen corner cases of dubious value. Just IMO, of course :-)
He’s doing great work, for sure. Initially I posted a lot of static analysis ‘fixes’, purely because I didn’t know how (entirely reasonably) picky you wanted to be about what to consider relevant. There has not been an endless stream of them: I’ve stalled on this Coverity work (on GTK+ at least) since the end of 2013. Since the first round of patches, I have tried to be more selective about which issues I patch, and have marked a lot more as false positives in Coverity. I believe the patches currently on bug #712770 (and a couple of GLib bugs) are all now mostly relevant bugs which can be triggered in the real world. For example, this one’s quite similar to one of the problems Maks fixed: https://bugzilla.gnome.org/show_bug.cgi?id=712760#c103 This one is triggerable (IIRC) by calling gtk_widget_set_visual(widget, NULL): https://bugzilla.gnome.org/show_bug.cgi?id=712760#c106 This one fixes handling NULL page setup objects when printing: https://bugzilla.gnome.org/show_bug.cgi?id=712760#c107 If you still think the balance is wrong, let me know, and we can try and sort out some rules for what kinds of issues to consider patching. I believe that fixing static analysis errors (or marking them as ignored) is like fixing compiler warnings: a lot of fiddling around in the beginning to get the code clean, and then the value comes from seeing new errors come in with new changes. Please, either review the patches; or reject them all with a big notice somewhere saying that GLib is not expected to be Coverity-clean and I’ll stop wasting my time trying to make static analysis useful. Either outcome would be fine. Just don’t leave the patches unreviewed indefinitely. That’s the worst of all worlds. Philip
Attachment:
signature.asc
Description: This is a digitally signed message part