[Geany-devel] Ideas on increasing quality of plugins

[Lex Trotman]

Hi Colomban,

Agree with most, comment on the rest below.

On 9 March 2011 14:00, Colomban Wendling <lists.ban at herbesfolles.org> wrote:
> 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);

Agree for shadow of locals and parameters, agree for globals but only
because all globals should be prefixed by a module prefix like the
Geany_ symbols are.  Is there any way to check this?

> * -Waggregate-return because returning aggregate values is no good for
> performances and it's not a Good Practice in C;

Why isn't it good?  Performance should be as good if not better than
returning multiple parameters via pointers and then assembling the
aggregate in the caller.  Why isn't it Good Practice?? (your caps :-)
Its been standard ever since someone invented allocating the return
variable on the stack prior to the call, so it can be referenced
relative to the frame pointer.

> * -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?

I'd agree that "mostly" strings should be treated as const, but
sometimes they are a template in which code replaces characters with
values.  I havn't looked at the plugins but I'd guess that most of the
warnings are from initialising char * instead of const char *.  Us C++
programmers get used to using const char * for string literals.  For
the few that really do mean to be written to, initialising a char[]
works fine.

>
> 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...

IIUC prototypes is C, declarations is C++ because it allows some
situations that C++ explicitly requires direct definition eg in
templates or inlines.

Cheers
Lex

>
>> 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
> _______________________________________________
> Geany-devel mailing list
> Geany-devel at uvena.de
> http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
>