Hi All,
On the off chance that we might be converging on a release I did a compile of the plugins with the usual options (-Wall -Wextra -Wno-unused-paratemers).
This was on GTK2 since many still don't work for GTK3.
The results are not too bad really :)
To summarise, other than deprecations which will be discussed below:
codenav - 2 unused variables debugger - 1 warning, useless test, probably inconsequential geanygendoc - 2 warnings, extra switch case may be consequential, unused function geanysendmail - 3 const discarded warnings multiterm - unused variable warnings in code generated by the valac, and const discarded warnings
Unused functions and variables are probably inconsequential, unless they are the result of a typo in the code meant to use them, they need a quick check and correct.
Extra switch case may indicate a typo, needs checking.
Discarded const potentially is a problem, it may indicate that something may try to modify literal strings, which may have undefined behaviour. Sometimes library headers use char* when they should have const char*, this is annoying, but it can usually be worked around. But must be checked, including the ones generated by the Vala compiler.
The details are at http://pastebin.geany.org/76dfx/
Since this is with GTK2 there are no GTK deprecation warnings (except in Vala, which seems to have decided that the whole of GTK2 is deprecated), but Glib ones are starting to creep through, in threading mostly. The problem with using the replacement functions is that their existence depends on the version of glib on the target system, not on the system Geany is compiled on. Using the new functions may therefore result in nasty surprises for users of binary packages. Any suggestions anyone?
Cheers Lex
Le 20/02/2014 09:07, Lex Trotman a écrit :
[...] geanygendoc - 2 warnings, extra switch case may be consequential, unused function [...]
Unused functions and variables are probably inconsequential, unless they are the result of a typo in the code meant to use them, they need a quick check and correct.
In geanygendoc, it was simply dead code because of a quick fix/workaround. Removed, since anyway that code, while providing interesting (unused) feature, wasn't working properly. And well, we have a VCS anyway to get back removed stuff if needed.
Extra switch case may indicate a typo, needs checking.
But actually it doesn't. It's due to the fact g_scanner_get_next_token() returns an enumeration type, while actually it should probably be an integer. This function returns the next token type, and this can be any character, plus a few extra, and is implemented as an enumeration with a gap for naked bytes:
enum { /* few named bytes for convenience */ EOF = 0; BRACE_OPEN = '{'; BRACE_CLOSE = '}'; /* ... */ EXTRA_STUFF = 256; /* ... */ }
This relies on the enumeration type to be implemented as an integer and implicitly accept intermediate values.
I "fixed" this by casting the return type to integer, but now in place of
ggd-file-type-loader.c: In function 'ggd_file_type_read_match': ggd-file-type-loader.c:412:11: warning: case value '46' not in enumerated type 'GTokenType' [-Wswitch] case '.': /* skip it */ break;
I get
ggd-file-type-loader.c: In function 'ggd_file_type_read_match': ggd-file-type-loader.c:411:17: warning: cast from function call of type 'GTokenType' to non-matching type 'unsigned int' [-Wbad-function-cast] switch ((guint) g_scanner_get_next_token (scanner)) {
way better, right? So what can I tell you, get the GLib fixed? :)
Cheers, Colomban
On 21 February 2014 02:02, Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 20/02/2014 09:07, Lex Trotman a écrit :
[...] geanygendoc - 2 warnings, extra switch case may be consequential, unused function [...]
Unused functions and variables are probably inconsequential, unless they are the result of a typo in the code meant to use them, they need a quick check and correct.
In geanygendoc, it was simply dead code because of a quick fix/workaround. Removed, since anyway that code, while providing interesting (unused) feature, wasn't working properly. And well, we have a VCS anyway to get back removed stuff if needed.
Extra switch case may indicate a typo, needs checking.
But actually it doesn't. It's due to the fact g_scanner_get_next_token() returns an enumeration type, while actually it should probably be an integer. This function returns the next token type, and this can be any character, plus a few extra, and is implemented as an enumeration with a gap for naked bytes:
enum { /* few named bytes for convenience */ EOF = 0; BRACE_OPEN = '{'; BRACE_CLOSE = '}'; /* ... */ EXTRA_STUFF = 256; /* ... */ }
This relies on the enumeration type to be implemented as an integer and implicitly accept intermediate values.
I "fixed" this by casting the return type to integer, but now in place of
ggd-file-type-loader.c: In function 'ggd_file_type_read_match': ggd-file-type-loader.c:412:11: warning: case value '46' not in enumerated type 'GTokenType' [-Wswitch] case '.': /* skip it */ break;
I get
ggd-file-type-loader.c: In function 'ggd_file_type_read_match': ggd-file-type-loader.c:411:17: warning: cast from function call of type 'GTokenType' to non-matching type 'unsigned int' [-Wbad-function-cast] switch ((guint) g_scanner_get_next_token (scanner)) {
way better, right? So what can I tell you, get the GLib fixed? :)
You should write CTPL, C Token Parsing Library, and do it right ... what do you mean, the names been taken?
Oh ok, do it the way discussed on IRC :)
Cheers Lex
Cheers, Colomban _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 14-02-20 12:07 AM, Lex Trotman wrote:
Hi All,
On the off chance that we might be converging on a release I did a compile of the plugins with the usual options (-Wall -Wextra -Wno-unused-paratemers).
This was on GTK2 since many still don't work for GTK3.
The results are not too bad really :)
To summarise, other than deprecations which will be discussed below:
codenav - 2 unused variables debugger - 1 warning, useless test, probably inconsequential geanygendoc - 2 warnings, extra switch case may be consequential, unused function geanysendmail - 3 const discarded warnings multiterm - unused variable warnings in code generated by the valac, and const discarded warnings
Unused functions and variables are probably inconsequential, unless they are the result of a typo in the code meant to use them, they need a quick check and correct.
Extra switch case may indicate a typo, needs checking.
Discarded const potentially is a problem, it may indicate that something may try to modify literal strings, which may have undefined behaviour. Sometimes library headers use char* when they should have const char*, this is annoying, but it can usually be worked around. But must be checked, including the ones generated by the Vala compiler.
No it mustn't :) I won't check or fix warnings in Vala's generated code, at least not as part of the Geany project. If it helps you out, just pretend the generated C code is like GCC's generated ASM code and that you assembled this code with very strong warnings and someone suggested you sift through GCC's generated assembler to verify each warning. I'm not opposed to any patches you provide though, if you'd like to read through the generated code and provide patches upstream and if they require changes in the plugin, apply them.
IMO, we should just disable warnings on Vala's generated code altogether and rely only on the Vala warnings. Anything else is a Vala bug and not our problem. I would've done this long ago but I'm not sure how to properly/portably disable warnings for a particular target in Autotools.
The details are at http://pastebin.geany.org/76dfx/
Since this is with GTK2 there are no GTK deprecation warnings (except in Vala, which seems to have decided that the whole of GTK2 is deprecated), [snip]
The GTK2 Vala binding is deprecated, same as the GTK2 C API/library. There is a compiler flag for Valac to make it disable the deprecation warnings, if that is desirable.
Cheers, Matthew Brush
[...]
Well, the same as for code written by humans, the C compiler warning is telling us something *might* be wrong with the code. The Vala compiler may have bugs, so such things need checking the same as manual code, to avoid possible UB. If, as in these cases, it seems to be ok, then its just noise, but unless you check you don't know.
IMO, we should just disable warnings on Vala's generated code altogether and rely only on the Vala warnings. Anything else is a Vala bug and not our problem. I would've done this long ago but I'm not sure how to properly/portably disable warnings for a particular target in Autotools.
Whilst I don't agree totally (see above), thats probably better, but I dunno how to do it either, Autotools experts please assist.
[...]
The GTK2 Vala binding is deprecated, same as the GTK2 C API/library. There is a compiler flag for Valac to make it disable the deprecation warnings, if that is desirable.
Probably, its just meaningless noise.
Cheers Lex
Cheers, Matthew Brush
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
Le 21/02/2014 03:16, Lex Trotman a écrit :
[...]
Well, the same as for code written by humans, the C compiler warning is telling us something *might* be wrong with the code. The Vala compiler may have bugs, so such things need checking the same as manual code, to avoid possible UB. If, as in these cases, it seems to be ok, then its just noise, but unless you check you don't know.
IMO, we should just disable warnings on Vala's generated code altogether and rely only on the Vala warnings. Anything else is a Vala bug and not our problem. I would've done this long ago but I'm not sure how to properly/portably disable warnings for a particular target in Autotools.
Whilst I don't agree totally (see above), thats probably better, but I dunno how to do it either, Autotools experts please assist.
Per-target flags are easy, they are target_CFLAGS, target_LDFLAGS and so on. Thought, note this is per *target*, so if you mix C and Vala in the same target, obviously those flags will apply to all use of the C compiler, linker, etc. for this target.
To disable warnings, there are 2 solutions:
1) don't add the warning flags; 2) add flags to disable the warnings.
Solution 1) seems reasonable, but it won't be able to remove user-set warnings in CFLAGS -- and we just *cannot* ignore user CFLAGS, there may be include paths and stuff.
Also, with the current setup it seems easier to just add flags than remove existing ones, because currently they are added automatically in vars.build.mk. Adding them conditionally may be tricky, because AFAIK make conditionals are unportable (e.g. BSD make and GNU make aren't compatible). Maybe Loong Jin knows better?
Even if we go with solution 2), we have 2 ways to apply it: overriding some user warnings flags or not. Overriding user warning flags would allow for users to have stricter warnings for real C code while not being flooded by Vela-generated ones, but again, it may be considered harmful. OTOH, if we don't override it basically only benefit people not having a custom CFLAGS defined (even if it is set to the same value as our default).
Attached are 2 examples of solution 2), not overriding and overriding. Just toy patches for now, but it should work.
[...]
The GTK2 Vala binding is deprecated, same as the GTK2 C API/library. There is a compiler flag for Valac to make it disable the deprecation warnings, if that is desirable.
Is there really a flag to only disable deprecated warnings? IIRC there is only --disable-warnings, and IIUC it removes everything not an error?
Cheers, Colomban
On 14-02-21 06:30 AM, Colomban Wendling wrote:
Le 21/02/2014 03:16, Lex Trotman a écrit :
[...]
Well, the same as for code written by humans, the C compiler warning is telling us something *might* be wrong with the code. The Vala compiler may have bugs, so such things need checking the same as manual code, to avoid possible UB. If, as in these cases, it seems to be ok, then its just noise, but unless you check you don't know.
IMO, we should just disable warnings on Vala's generated code altogether and rely only on the Vala warnings. Anything else is a Vala bug and not our problem. I would've done this long ago but I'm not sure how to properly/portably disable warnings for a particular target in Autotools.
Whilst I don't agree totally (see above), thats probably better, but I dunno how to do it either, Autotools experts please assist.
Per-target flags are easy, they are target_CFLAGS, target_LDFLAGS and so on. Thought, note this is per *target*, so if you mix C and Vala in the same target, obviously those flags will apply to all use of the C compiler, linker, etc. for this target.
To disable warnings, there are 2 solutions:
- don't add the warning flags;
- add flags to disable the warnings.
Solution 1) seems reasonable, but it won't be able to remove user-set warnings in CFLAGS -- and we just *cannot* ignore user CFLAGS, there may be include paths and stuff.
Also, with the current setup it seems easier to just add flags than remove existing ones, because currently they are added automatically in vars.build.mk. Adding them conditionally may be tricky, because AFAIK make conditionals are unportable (e.g. BSD make and GNU make aren't compatible). Maybe Loong Jin knows better?
Even if we go with solution 2), we have 2 ways to apply it: overriding some user warnings flags or not. Overriding user warning flags would allow for users to have stricter warnings for real C code while not being flooded by Vela-generated ones, but again, it may be considered harmful. OTOH, if we don't override it basically only benefit people not having a custom CFLAGS defined (even if it is set to the same value as our default).
Attached are 2 examples of solution 2), not overriding and overriding. Just toy patches for now, but it should work.
OK, I didn't think it was OK to assume GCC was the compiler, so I wasn't sure how you'd portably (across compilers) tell the compiler flags without doing a bunch of configure checks to see whether the compiler accepted such flags or something. My GCC/Clang-specific idea was to maybe just use something like `-Xcc=-w` or whatever in one of the VALAFLAGS variables to disable all warnings related to Vala code, though I'm not sure if Autotools invokes the C compiler via Valac or independently.
[...]
The GTK2 Vala binding is deprecated, same as the GTK2 C API/library. There is a compiler flag for Valac to make it disable the deprecation warnings, if that is desirable.
Is there really a flag to only disable deprecated warnings? IIRC there is only --disable-warnings, and IIUC it removes everything not an error?
I believe Valac's --disable-deprecated option will disable it warning about deprecated annotations in the VAPI files. I'd have to test it to be sure though.
Cheers, Matthew Brush
Le 21/02/2014 16:39, Matthew Brush a écrit :
On 14-02-21 06:30 AM, Colomban Wendling wrote:
Le 21/02/2014 03:16, Lex Trotman a écrit :
[...]
OK, I didn't think it was OK to assume GCC was the compiler,
It isn't, indeed. Although well, to be fair we probably have no idea what happens if it's not GCC-like. Or maybe I even remember there was problems with MSVC?
so I wasn't sure how you'd portably (across compilers) tell the compiler flags without doing a bunch of configure checks to see whether the compiler accepted such flags or something.
Well, basically, you can't. Hence the bunch of configure checks -- although the ones we already have are only for GCC flags, because I didn't know what other compilers used.
My GCC/Clang-specific idea was to maybe just use something like `-Xcc=-w` or whatever in one of the VALAFLAGS variables to disable all warnings related to Vala code, though I'm not sure if Autotools invokes the C compiler via Valac or independently.
AFAIK, Autotools generate C source from vala, and the build the C sources. It's pretty obvious when you see it adds C sources to the distribution I guess :)
[...]
The GTK2 Vala binding is deprecated, same as the GTK2 C API/library. There is a compiler flag for Valac to make it disable the deprecation warnings, if that is desirable.
Is there really a flag to only disable deprecated warnings? IIRC there is only --disable-warnings, and IIUC it removes everything not an error?
I believe Valac's --disable-deprecated option will disable it warning about deprecated annotations in the VAPI files. I'd have to test it to be sure though.
Oh, I believe it disabled all deprecated features -- rather the contrary then. But I'm not sure either.
Cheers, Colomban
On Fri, 21 Feb 2014 16:52:51 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
[...]
OK, I didn't think it was OK to assume GCC was the compiler,
It isn't, indeed. Although well, to be fair we probably have no idea what happens if it's not GCC-like. Or maybe I even remember there was problems with MSVC?
We disabled MSVC some time ago (see "geany-plugins fail to build with msvc"). In short, cl recognices "template" as C keyword :) and the CFLAGS / LDFLAGS obtained from pkg-config under win~1 are suitable for gcc, not MSVC.
On 22 February 2014 04:50, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Fri, 21 Feb 2014 16:52:51 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
[...]
OK, I didn't think it was OK to assume GCC was the compiler,
It isn't, indeed. Although well, to be fair we probably have no idea what happens if it's not GCC-like. Or maybe I even remember there was problems with MSVC?
We disabled MSVC some time ago (see "geany-plugins fail to build with msvc"). In short, cl recognices "template" as C keyword :)
I thought Matthew fixed that so we could use C++ plugins.
and the
CFLAGS / LDFLAGS obtained from pkg-config under win~1 are suitable for gcc, not MSVC.
But that is indeed a problem, presumably because things like gtk binaries have been cross compiled for win, not with msvc. Also is it still true that g++ and msvc++ binaries can't be linked?
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 14-02-21 11:54 AM, Lex Trotman wrote:
On 22 February 2014 04:50, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On Fri, 21 Feb 2014 16:52:51 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
[...]
OK, I didn't think it was OK to assume GCC was the compiler,
It isn't, indeed. Although well, to be fair we probably have no idea what happens if it's not GCC-like. Or maybe I even remember there was problems with MSVC?
I guess it would be the same result as passing something like `-Wl,--export-dynamic` on platforms that don't support it (ie. compilation/linking failure). Even a GCC-clone like Clang has some different options, for example to limit errors is `-fmax-errors=N` on GCC and `-ferror-limit=N` on Clang (at least last time I tried). So I guess even if a compiler is GCC-like, passing an invalid `-Wfoo` might be bad, if not fatal.
MSVC is a totally different story WRT to flags of course :)
We disabled MSVC some time ago (see "geany-plugins fail to build with msvc"). In short, cl recognices "template" as C keyword :)
I thought Matthew fixed that so we could use C++ plugins.
Only in the headers, if 'template' is still used in source files it would choke a compiler that thought it was a C keyword.
and the
CFLAGS / LDFLAGS obtained from pkg-config under win~1 are suitable for gcc, not MSVC.
pkg-config on Windows supports an option called `--msvc-syntax` which presumably outputs CL-compatible flags.
But that is indeed a problem, presumably because things like gtk binaries have been cross compiled for win, not with msvc. Also is it
It shouldn't matter, C has a defined ABI AFAIK.
still true that g++ and msvc++ binaries can't be linked?
I think just because of C++ not having a defined ABI and compilers choosing to implement name-mangling and such differently. I don't think it matters for Geany though since nothing external would be linked with C++ symbols, even Scintilla's exposed stuff is plain/extern C AFAIK.
Cheers, Matthew Brush
On Fri, 21 Feb 2014 19:50:39 +0200 Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
We disabled MSVC some time ago (see "geany-plugins fail to build with msvc").
Yepp. But don't remember the details ;)
Cheers, Frnak
On Thu, 20 Feb 2014 19:07:04 +1100 Lex Trotman elextr@gmail.com wrote:
geanysendmail - 3 const discarded warnings
I have these on my plate, but no idea whether this is really a problem (I don't think so) nor how I can fix it.
Cheers, Frank
Le 21/02/2014 15:02, Frank Lanitz a écrit :
On Thu, 20 Feb 2014 19:07:04 +1100 Lex Trotman elextr@gmail.com wrote:
geanysendmail - 3 const discarded warnings
I have these on my plate, but no idea whether this is really a problem (I don't think so)
It's not a real problem, it's only a problem of that GTK API.
nor how I can fix it.
Well, apart from adding ugly explicit casts to hide the warnings, you can't do much.
Cheers, Colomban
Hi Frank,
Sigh, this is one of the places where a system interface should be const char* but isn't.
Quick and dirty patch attached.
Cheers Lex
On 22 February 2014 01:02, Frank Lanitz frank@frank.uvena.de wrote:
On Thu, 20 Feb 2014 19:07:04 +1100 Lex Trotman elextr@gmail.com wrote:
geanysendmail - 3 const discarded warnings
I have these on my plate, but no idea whether this is really a problem (I don't think so) nor how I can fix it.
Cheers, Frank
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Sat, 22 Feb 2014 07:06:43 +1100 Lex Trotman elextr@gmail.com wrote:
Sigh, this is one of the places where a system interface should be const char* but isn't.
Quick and dirty patch attached.
Thanks. Will apply it.
Cheers, Frank