On 13-11-2013 23:58, Alex Valavanis wrote:

Hi All,

Has anyone played around with the Clang analyzer much [1]? I gave it a quick run last night, and it picked up a few hundred potential issues in our code. At first glance, a lot of them are probably harmless, but the tool is very good at highlighting places where we could be referencing NULL pointers or using uninitialised variables.

Even when we "know" that the highlighted code is safe (e.g., pointers have implicitly been checked before referencing), we can add assertions to make the safety explicit. Here's an example [2]

I'd strongly recommend that we make use of this tool!

AV

[1] http://clang-analyzer.llvm.org/scan-build.html [2] http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12798

Great that you are using these tools! Indeed they really help.

One comment: don't sprinkle g_asserts everywhere, where we can also nicely deal with the ptr being NULL. We don't disable g_assert in releases, so you are basically adding crashers (of stuff that would crash anyway), instead of resolving them ;-) (nullptr crashes are usually very easy to track down, so the g_assert doesn't do much for such cases IMHO)

For example, this code fragment from splivarot.cpp

if (item == NULL || ( !SP_IS_SHAPE(item) && !SP_IS_TEXT(item) ) ) { desktop->messageStack()->flash(Inkscape::ERROR_MESSAGE, _("...")); return; } if (SP_IS_SHAPE(item)) { curve = SP_SHAPE(item)->getCurve(); if (curve == NULL) { return; } } if (SP_IS_TEXT(item)) { curve = SP_TEXT(item)->getNormalizedBpath(); if (curve == NULL) { return; } } g_assert(curve != NULL);

could be changed to:

if (item == NULL || ( !SP_IS_SHAPE(item) && !SP_IS_TEXT(item) ) ) { desktop->messageStack()->flash(Inkscape::ERROR_MESSAGE, _("...")); return; } else if (SP_IS_SHAPE(item)) { curve = SP_SHAPE(item)->getCurve(); } else { // the item must be SP_TEXT curve = SP_TEXT(item)->getNormalizedBpath(); } if (!curve) { return; }

which is more logical to me, and does not crash unnecessarily.

Cheers, Johan