[Geany-devel] Ideas on increasing quality of plugins

[Colomban Wendling]

Le 08/03/2011 22:29, Colomban Wendling a écrit :
> Le 08/03/2011 19:58, Enrico Tröger a écrit :
>> On Tue, 08 Mar 2011 11:06:16 +0100, Frank wrote:
>>
>>> Am 23.02.2011 01:10, schrieb Matthew Brush:
>>>> For first thing, maybe we could enforce use/passing of those tools
>>>> mentioned and these before adding to release, examples:
>>>> http://www.splint.org/
>>>> http://valgrind.org/info/tools.html
>>>> (suppression for GTK -
>>>> http://people.gnome.org/~johan/gtk.suppression)
>>>> http://www.gnu.org/software/indent/ (just for making coding styles
>>>> more consistent between plugins) http://check.sourceforge.net/ or
>>>> http://cutest.sourceforge.net/ or http://cunit.sourceforge.net/
>>>> Perhaps some or all of these could be automated.
> 
> I don't suggest (yet) to use splint, because I haven't found it useful,
> it reported ways too much things (and some was false positive) to be
> really usable. However, I probably don't know how to configure it (and
> haven't the time to find out yet), so if somebody with experience with
> it can provide some hints, maybe we can add this too.
> 
> For Valgrind however, I doubt we can do anything automatically, because
> it's a runtime checker, and its output must generally be read with some
> care. Writing some sort of guidelines and howto is probably the way to go.
> 
>>>
>>> I like that idea. Can someone of you build up a howto on how to use it?
>>> I did try valgrind in past and wished for some advice ;)
>>>
>>> One this is done we can think of automatic tests with some of this
>>> tools.
>>
>> I, and obviously, Colomban as well, though indepdent from each other :D,
>> recently played[1,2] with cppcheck. A small tool for static code
>> analysis which actually found a few things in the geany-plugins
>> repository.
>>
>> As I'm currently reworking the system to create the nightly builds, we
>> could integrate such checks into the nightly builds, maybe run cppcheck
>> on the sources after the builds and present the results somewhere on
>> nightly.geany.org.
> I think it's a good idea.
> 
> I did a few checks, and this is what I suggest:
> 1) run cppcheck on `make check` and abort if it detects an error;
> 2) enable by default (though in a portable manner) some compiler flags
> such as -Werror-implicit-function-declaration [1] [2]
After a little more thinking and testing, I suggest:

* -Werror=implicit-function-declaration (see above);
* -Werror=pointer-arith, to avoid portability issues with untyped
pointer arithmetic;
* -Wundef because preprocessor tests should rely on defined variables or
explicitly use ifdef;
* -Wshadow because shadowing symbols is a Bad Practice (and maybe it is
even non-standard, not quite sure however);
* -Waggregate-return because returning aggregate values is no good for
performances and it's not a Good Practice in C;
* -Wwrite-strings because using a string literal as a modifiable value
may lead to crashes, and anyway good code should probably assume it's
constant.

I thing all is perfectly fine but maybe -Wwrite-strings. What
-Wwrite-strings does is set all string literals ("foobar") constant
(const char*). It is a good practice to assume string literals are
constant, but however it is not necessarily wrong to assign it to a
non-constant variable (if one actually don't modify the value), so it
may have false positive.
And actually it probably has: it adds lots of warnings in some plugins,
and though I haven't checked, I doubt all are true errors.
I think it'd be a good thing if no plugin would emit a warning using
this, but maybe it's too soon for this, or maybe we don't want this at
all, not sure. So, do you think it is or not a good idea to use it?

And finally, I though of -Wstrict-prototypes, -Wmissing-prototypes and
-Wmissing-declarations [1], but...

* -Wstrict-prototypes even emits a warning in a GTK+ header and one in
Scintilla.h;
* -Wmissing-prototypes (and -Wmissing-declarations) emits a warning for
plugin_init() and friends (though I thought it was fixed already?).

...So we probably don't want them by default. Moreover, the problem
reported by -Wmissing-prototypes is not really important for preventing
crashes, it's mostly a "beauty" thing.

[1] not sure of the difference with -Wmissing-prototypes...

> Maybe some other tests might be good, but I think this is a start.
> 
> I'm working on implementing this for Autotools, maybe I can take a look
> at Waf too (but Enrico would love to do this :D).
> 
> Thoughts?

Cheers,
Colomban