On 10 March 2011 00:45, Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 09/03/2011 05:31, Lex Trotman a écrit :
Hi Colomban,
Agree with most, comment on the rest below.
On 9 March 2011 14:00, Colomban Wendling lists.ban@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:
- run cppcheck on `make check` and abort if it detects an error;
- 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?
Not that I know, unfortunately. However maybe a dumb sed/grep might do the trick.
- -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.
Well... I must admit I'm not really pro on the subject, but AFAIK the main problem is ABI, because since the return value will not necessarily fit in registers the compiler is likely to generate a dummy parameter to the function (as you might have done yourself), but this implicitly, so changing the "natural" ABI of the function.
Functions that return too much to fit in the registers should use the stdcall ABI which defines an extra reference parameter for the aggregate. (Or rather the compiler should use it). There is no such thing as a "natural" ABI that is corrupted by aggregate returns, there are ABIs defined by Intel and other CPU makers for all the required call/return patterns. Yes its slower than returning something in a register, but if you have got to return an aggregate value thats too big you have to return it. Might as well let the compiler do the heavy lifting rather than having to declare a temporary and pass a pointer to it as an explicit parameter or otherwise return the components and assemble the aggregate in the caller.
Back in the dim distant past (1970s) when function prototypes were as rare as hens teeth such returns were a drama since they required a prototype. I don't see why we should continue this restriction as a hangover from the dinosaurs and the voracious EGCS compiler raptor :-).
Not that the restriction would have much impact, a survey found about 95% of C code returned void, some integer type or some pointer type, probably because thats the way the libraries are written and everyone blindly uses the same style. But why impose an arbitrary restriction on standard compliant code? Someone who happened to want to use a more pure functional coding style might even produce better code than the side effect ridden normal C style. (At least thats what the smart PHDs seem to indicate).
Any'ow I'll climb down off me soapbox now.
However it's true that since we don't speak about libraries, we don't mind about that.
As I said above, there is a defined call ABI for functions returning aggregates, all such functions should use it, libraries or not.
Another point seems to be that some ooold compilers used not to support aggregate return, though once again we probably don't mind.
Well then they are not standard compliant, so I agree they don't matter :-)
OTOH, I don't know of any good (or not BTW) code using aggregates as return values; maybe there's other reasons?
Habit, see above :-)
Cheers Lex
PS IIUC most current compilers will compile
struct big_aggregate a = f();
to pass a as the return parameter directly so no copy happens. That used to be another objection to returning aggregates.
However, I don't really mind about this one, since even if I used to think it's a performance issue, it's not a really important point. We're trying to make code stronger, not necessarily faster :D
- -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.
Yes, none of the warnings that this flag would trigger are either hard or wrong to fix. I personally think it's worth using it, just wanted to see if concerned people would care to fix these.
Cheers, Colomban
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@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel