Hi,
I'm used to compile my programs with quite strict compiler options [1], and it made me see that the plugin API had some non-const arguments or field where it probably should have const ones - where string literals are most likely expected. I think of keybindings_set_item(), plugin_signal_connect(), the PluginInfo structure, etc. I think it is quite important since it makes the plugin API cleaner (IMHO) and allows to write more clean plugins (no more useless implicit promotions that may even be invalid). Attached a patch with the suggested changes.
Although this changes some API code, I think it is completely API and ABI [2] compatible: changes only makes the API more permissive on given types.
On quite the same topic, would a patch that fixes a lot of small problems like that in Geany's core be appreciated? It isn't hard to do but I think it would clean the code a bit.
Regards, Colomban
[1] -Wall -W -O2 -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic [2] even though I don't know so much about ABI things, I doubt the const information would ever be part of it - at least on x86.
On Wed, 21 Apr 2010 22:51:33 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Hi,
I'm used to compile my programs with quite strict compiler options [1], and it made me see that the plugin API had some non-const arguments or field where it probably should have const ones - where string literals are most likely expected. I think of keybindings_set_item(), plugin_signal_connect(), the PluginInfo structure, etc. I think it is quite important since it makes the plugin API cleaner (IMHO) and allows to write more clean plugins (no more useless implicit promotions that may even be invalid).
I suppose this is due to source "foo" strings being const.
Attached a patch with the suggested changes.
Applied, thanks.
Although this changes some API code, I think it is completely API and ABI [2] compatible: changes only makes the API more permissive on given types.
On quite the same topic, would a patch that fixes a lot of small problems like that in Geany's core be appreciated? It isn't hard to do but I think it would clean the code a bit.
Yes, but only if it's clear that const is really meant, not just to silence compiler warnings.
I don't like casting away const (e.g. keybindings_set_item) but for the API it's OK when 'safe'.
[1] -Wall -W -O2 -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic
BTW, which one causes the const literal string warnings?
(You might want to add -ansi or -std=foo for portability.)
Regards, Nick
Nick Treleaven a écrit :
On Wed, 21 Apr 2010 22:51:33 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Hi,
I'm used to compile my programs with quite strict compiler options [1], and it made me see that the plugin API had some non-const arguments or field where it probably should have const ones - where string literals are most likely expected. I think of keybindings_set_item(), plugin_signal_connect(), the PluginInfo structure, etc. I think it is quite important since it makes the plugin API cleaner (IMHO) and allows to write more clean plugins (no more useless implicit promotions that may even be invalid).
I suppose this is due to source "foo" strings being const.
Exactly.
Attached a patch with the suggested changes.
Applied, thanks.
No problem :)
On quite the same topic, would a patch that fixes a lot of small problems like that in Geany's core be appreciated? It isn't hard to do but I think it would clean the code a bit.
Yes, but only if it's clear that const is really meant, not just to silence compiler warnings.
Of course. Casting away should only be done where the cast is completely wanted or at least carefully understood. I'll then do it unless somebody else go faster than me.
I don't like casting away const (e.g. keybindings_set_item) but for the API it's OK when 'safe'.
I'd wanted to make the internal pointer constant at the beginning, but I saw that it is assigned to non-const strings if group->plugin is set, so I searched a little the code before doing the cast. And it is because I don't think such a cast is a normal situation that I added a small comment.
[1] -Wall -W -O2 -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic
BTW, which one causes the const literal string warnings?
-Wwrite-strings. It makes string literals explicitly constant so such an implicit cast is found.
(You might want to add -ansi or -std=foo for portability.)
Yay true, even though most of the things that wouldn't be valid in ANSI C already gets a warning with the options above (rather that simply failing, I think e.g. of C++-style comments) with the current GCC default.
Ah, as we talk about compiler options, another think I like to add is -Werror-implicit-function-declaration (that, as its name suggests, take an implicit function declaration as an error), since such a situation is most likely a bug in the program; and passing invalid arguments to functions leads to strange bugs too anyway.
Regards, Colomban
On Thu, 22 Apr 2010 15:35:17 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
[1] -Wall -W -O2 -Wunused -Wunreachable-code -Wformat=2 -Wundef -Wshadow -Wpointer-arith -Wbad-function-cast -Wwrite-strings -Wconversion -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wmissing-declarations -Wmissing-noreturn -Wmissing-format-attribute -Wredundant-decls -Wnested-externs -pedantic
BTW, which one causes the const literal string warnings?
-Wwrite-strings. It makes string literals explicitly constant so such an implicit cast is found.
Oh thanks, I scanned through for a string warning but there must be a bug in my brain's grep function ;-)
Ah, as we talk about compiler options, another think I like to add is -Werror-implicit-function-declaration (that, as its name suggests, take an implicit function declaration as an error), since such a situation is most likely a bug in the program; and passing invalid arguments to functions leads to strange bugs too anyway.
It's already enabled for -Wall, according to: http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options
Regards, Nick
Ah, as we talk about compiler options, another think I like to add is -Werror-implicit-function-declaration (that, as its name suggests, take an implicit function declaration as an error), since such a situation is most likely a bug in the program; and passing invalid arguments to functions leads to strange bugs too anyway.
It's already enabled for -Wall, according to: http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options
Not exactly: I speak of the option that treats this as an error (and then aborts the compilation). As I already said, I think that such a warning is important enough to become an error.
On Thu, 22 Apr 2010 18:07:56 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
Ah, as we talk about compiler options, another think I like to add is -Werror-implicit-function-declaration (that, as its name suggests, take an implicit function declaration as an error), since such a situation is most likely a bug in the program; and passing invalid arguments to functions leads to strange bugs too anyway.
It's already enabled for -Wall, according to: http://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options
Not exactly: I speak of the option that treats this as an error (and then aborts the compilation). As I already said, I think that such a warning is important enough to become an error.
OK. Personally I always use -Werror and disable unused parameter warnings (for glade generated code).
Regards, Nick
On quite the same topic, would a patch that fixes a lot of small problems like that in Geany's core be appreciated? It isn't hard to do but I think it would clean the code a bit.
Yes, but only if it's clear that const is really meant, not just to silence compiler warnings.
Of course. Casting away should only be done where the cast is completely wanted or at least carefully understood. I'll then do it unless somebody else go faster than me.
I've took a look and fixed some warnings around and other small stuff. They are consts, a little uint vs. int when I'm sure it makes sense, staticize a few local functions, and so on.
I've found 4 things that I haven't fixed/touched: * build.c:174: This type of assignations aren't valid C90 (it's a C99 addition I think). Not sure it should be fixed or not since it gives a little gain - being sure the affected element is the right one. * document.c:473: The function document_stop_file_monitoring() has no prototype and isn't static, but the name made me think it is perhaps meant to be external for some reason and only misses its prototype? * main.c:1212+: What's that malloc stuff for??? These functions aren't even used... * the stash_group_add_entry() and its widget_id. The thing is that when it is a string it should be constant, but when it is a widget it shouldn't be... perhaps it is fine as now.
There is 2 patches: one that modifies the Geany's core, and the other that modifies that core plugins [1].
Regards, Colomban
[1] there is a missing free() fix for the export plugin too.
On Thu, 22 Apr 2010 23:56:35 +0200, Colomban wrote:
On quite the same topic, would a patch that fixes a lot of small problems like that in Geany's core be appreciated? It isn't hard to do but I think it would clean the code a bit.
Yes, but only if it's clear that const is really meant, not just to silence compiler warnings.
Of course. Casting away should only be done where the cast is completely wanted or at least carefully understood. I'll then do it unless somebody else go faster than me.
I've took a look and fixed some warnings around and other small stuff. They are consts, a little uint vs. int when I'm sure it makes sense, staticize a few local functions, and so on.
I've found 4 things that I haven't fixed/touched:
- build.c:174: This type of assignations aren't valid C90 (it's a C99
addition I think). Not sure it should be fixed or not since it gives a little gain - being sure the affected element is the right one.
I think it should be fixed as we usually try to keep the core C89/C90 compatible as there are users using compilers which require it.
- document.c:473: The function document_stop_file_monitoring() has no
prototype and isn't static, but the name made me think it is perhaps meant to be external for some reason and only misses its prototype?
While developing the GIO file monitoring code, I used that function in dialogs.c but then removed it again. I'll make it static for now as it isn't used anywhere else than in document.c. If necessary, we can make it public later again.
- main.c:1212+: What's that malloc stuff for??? These functions aren't
even used...
http://geany.svn.sf.net/viewvc/geany?view=rev&revision=1596 and see http://lists.uvena.de/pipermail/geany/2007-June/001093.html for the discussion about it. Maybe we can remove rpl_malloc() because we replaced the malloc() with g_malloc () at the same time.
- the stash_group_add_entry() and its widget_id. The thing is that when
it is a string it should be constant, but when it is a widget it shouldn't be... perhaps it is fine as now.
I think this is ok but I'll leave this to Nick.
There is 2 patches: one that modifies the Geany's core, and the other that modifies that core plugins [1].
Cool, thanks a lot . Will apply the patches later today.
[1] there is a missing free() fix for the export plugin too.
Good catch, thanks.
Regards, Enrico
- main.c:1212+: What's that malloc stuff for??? These functions aren't
even used...
http://geany.svn.sf.net/viewvc/geany?view=rev&revision=1596 and see http://lists.uvena.de/pipermail/geany/2007-June/001093.html for the discussion about it. Maybe we can remove rpl_malloc() because we replaced the malloc() with g_malloc () at the same time.
Oh yes, this famous GNU malloc stuff... Anyway, this is not used anymore since rev 1961 [1] that dropped AC_PROG_MALLOC [2] if I'm right. And anyway, do you need the GNU-malloc style? I think it can be safely removed.
Cool, thanks a lot .
No problem :)
Regards, Colomban
[1] http://geany.svn.sf.net/viewvc/geany?view=rev&revision=1961 [2] http://www.gnu.org/software/autoconf/manual/html_node/Particular-Functions.h...
On Sun, 25 Apr 2010 14:25:30 +0200, Colomban wrote:
- main.c:1212+: What's that malloc stuff for??? These functions
aren't even used...
http://geany.svn.sf.net/viewvc/geany?view=rev&revision=1596 and see http://lists.uvena.de/pipermail/geany/2007-June/001093.html for the discussion about it. Maybe we can remove rpl_malloc() because we replaced the malloc() with g_malloc () at the same time.
Oh yes, this famous GNU malloc stuff... Anyway, this is not used anymore since rev 1961 [1] that dropped AC_PROG_MALLOC [2] if I'm right. And anyway, do you need the GNU-malloc style? I think it can be safely removed.
Ah, great. Will remove it.
Thanks.
Regards, Enrico
On Sun, 25 Apr 2010 17:54:03 +0200, Enrico wrote:
On Sun, 25 Apr 2010 14:25:30 +0200, Colomban wrote:
- main.c:1212+: What's that malloc stuff for??? These functions
aren't even used...
http://geany.svn.sf.net/viewvc/geany?view=rev&revision=1596 and see http://lists.uvena.de/pipermail/geany/2007-June/001093.html for the discussion about it. Maybe we can remove rpl_malloc() because we replaced the malloc() with g_malloc () at the same time.
Oh yes, this famous GNU malloc stuff... Anyway, this is not used anymore since rev 1961 [1] that dropped AC_PROG_MALLOC [2] if I'm right. And anyway, do you need the GNU-malloc style? I think it can be safely removed.
Ah, great. Will remove it.
Comitted together with your patches. Thanks again.
Regards, Enrico
On Thu, 22 Apr 2010 23:56:35 +0200 Colomban Wendling lists.ban@herbesfolles.org wrote:
- the stash_group_add_entry() and its widget_id. The thing is that when
it is a string it should be constant, but when it is a widget it shouldn't be... perhaps it is fine as now.
I'm not sure what to do about this really.
Regards, Nick