Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On Wed, Oct 26, 2016 at 08:40:02AM +0200, Florian Schmidt wrote: > On 10/25/2016 06:38 PM, Dominik Vogt wrote: > >Right. A solution must disable the warnings on any compilers and > >versions the developers use, and not break compilation anywhere. > > The way I generally do it is check for the compiler, and then define > a macro for gcc and for clang using those attributes. Admittedly, > that works, because for the stuff I work on, those two are all that > is expected to be ever used; something that probably cannot be said > about fvwm. > > In the end, this mail (and the one from yesterday) are outdated > anyway No, I'm still thinking about a possible solution that works without using potentially unportable headers. A colleague has come up with this: x = (x >= 0) ? (((unsigned) x) & 0x7fff) : - ((- (unsigned) x) & 0x7fff); Well, there's CARD16/INT16 defined in Xmd.h, maybe we should use them instead of the the C standard headers. > because I see you already found another solution via the > SUPPRESSED_UNUSED_VAR_WARNING macro, so disregard my comments. > But, since I'm curious: that macro doesn't have the problem of > potentially masking warnings about using uninitialized variables? I hope so, but that may depend on how clever the compiler is. > I guess the important difference is that you only use , not x > itself any more? Yes. Apparently when the pointer is used, Gcc thinks any value set may be used, but does not consider it a potentially uninitialised use. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On 10/25/2016 06:38 PM, Dominik Vogt wrote: Right. A solution must disable the warnings on any compilers and versions the developers use, and not break compilation anywhere. The way I generally do it is check for the compiler, and then define a macro for gcc and for clang using those attributes. Admittedly, that works, because for the stuff I work on, those two are all that is expected to be ever used; something that probably cannot be said about fvwm. In the end, this mail (and the one from yesterday) are outdated anyway because I see you already found another solution via the SUPPRESSED_UNUSED_VAR_WARNING macro, so disregard my comments. But, since I'm curious: that macro doesn't have the problem of potentially masking warnings about using uninitialized variables? I guess the important difference is that you only use , not x itself any more? Still, that's interesting. Florian
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On 10/22/2016 07:35 PM, Dominik Vogt wrote: And the least invasive way to prevent this is faking a read with the coid-cast. I assume marking the variable as potentially unused via __attribute__((unused)) or similar is undesirable because it depends on compiler-specific extensions? Then again, the warnings are also compiler-specific... Florian
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On Sat, Oct 22, 2016 at 10:04:39PM +0100, Thomas Adam wrote: > On Sat, Oct 22, 2016 at 07:55:01PM +0100, Dominik Vogt wrote: > > Yes, but that does not fix the warning. "x" is unused because of > > the dummy replacement of the function call. The compiler does not > > see that if "x = 0" is ever executed, Fxyz_some_func always has a > > non-empty definition. > > > > I've always used > > > > if (FEATURE) > > { > > ... > > } > > > > Instead of > > > > #if FEATURE > > ... > > #endif > > > > so that the conditional code is always compiled, even if the > > feature is disabled (so we would catch compile errors earlier). > > But in this case, it introduces a warning. > > Yes -- which makes this impossible to fix unless the code is changed to be > #ifdef'd out, rather than using 'if (FEATURE)', which makes things harder to > read anyway. In that case I'd rather have a warning in a rare corner case than the code becoming un-compileable over time because nobody ever bothers to disable all optional configure features before building a release. Ifdefs are a maintenance nightmare. But of course it is fixable, we'd just have to replace the dummy macros with real functions that do nothing. A decent compiler will remove the dead code anyway. But I won't do that unless I really find no better way. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On Sat, Oct 22, 2016 at 07:55:01PM +0100, Dominik Vogt wrote: > Yes, but that does not fix the warning. "x" is unused because of > the dummy replacement of the function call. The compiler does not > see that if "x = 0" is ever executed, Fxyz_some_func always has a > non-empty definition. > > I've always used > > if (FEATURE) > { > ... > } > > Instead of > > #if FEATURE > ... > #endif > > so that the conditional code is always compiled, even if the > feature is disabled (so we would catch compile errors earlier). > But in this case, it introduces a warning. Yes -- which makes this impossible to fix unless the code is changed to be #ifdef'd out, rather than using 'if (FEATURE)', which makes things harder to read anyway. -- Thomas Adam
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On Sat, Oct 22, 2016 at 06:19:46PM +0100, Thomas Adam wrote: > On Sat, Oct 22, 2016 at 03:42:13PM +0100, Dominik Vogt wrote: > > Proof reading this patch would also be helpful. > > I've taken a look. It's fine. I can't say I like the void casts all over the > place -- what's wrong with giving a default value? It results in "set but not used" messages (gcc-4.7.2). This is in some functions where a feature is disabled with a macro: void foo(void) { int x; if (!FEATURE_XYZ) { return; } x = 0; Fxyz_some_func(); return; } Where #if FEATURE_XYZ #define Fxyz_some_function(a) Xyz_some_sunction(a) #else #define Fxyz_some_function(a) do { } while (0) #fi If FEATURE_XYZ is disabled, the preprocessed code is just ... x = 0; do { } while (0); ... And the least invasive way to prevent this is faking a read with the coid-cast. > However, since I'm using FreeBSD and hence clang, here's a further diff I'd > like you to include (to shut up clang): > > diff --git a/fvwm/icons.c b/fvwm/icons.c > index a3cb0cd..a6cc234 100644 > --- a/fvwm/icons.c > +++ b/fvwm/icons.c > @@ -754,7 +754,7 @@ void CreateIconWindow(FvwmWindow *fw, int def_x, int > def_y) > /* when fvwm is using the non-default visual client > * supplied icon pixmaps are drawn in a window with no > * relief */ > - int off ; > + int off = 0; > > (void)off; > if (Pdefault || fw->iconDepth == 1 || Ouch. So, the void casts are really bad because they actualy hide real bugs. This is not just a warning fix, the patch really breaks code that was fine before, except for the warning. There must be another way then... > Incidentally, you can always check to see what the different compilers are > doing (gcc/clang) by looking at the output from the travis-ci builds. In the > Clang case, your build looks are here: > > https://travis-ci.org/fvwmorg/fvwm/jobs/169749072 Hm, I just see a summary on top and below the heading "Job log" a "loading" icon that never finishes. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
On Sat, Oct 22, 2016 at 03:42:13PM +0100, Dominik Vogt wrote: > Proof reading this patch would also be helpful. I've taken a look. It's fine. I can't say I like the void casts all over the place -- what's wrong with giving a default value? However, since I'm using FreeBSD and hence clang, here's a further diff I'd like you to include (to shut up clang): diff --git a/fvwm/icons.c b/fvwm/icons.c index a3cb0cd..a6cc234 100644 --- a/fvwm/icons.c +++ b/fvwm/icons.c @@ -754,7 +754,7 @@ void CreateIconWindow(FvwmWindow *fw, int def_x, int def_y) /* when fvwm is using the non-default visual client * supplied icon pixmaps are drawn in a window with no * relief */ - int off ; + int off = 0; (void)off; if (Pdefault || fw->iconDepth == 1 || Incidentally, you can always check to see what the different compilers are doing (gcc/clang) by looking at the output from the travis-ci builds. In the Clang case, your build looks are here: https://travis-ci.org/fvwmorg/fvwm/jobs/169749072 Kindly, Thomas
Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
Proof reading this patch would also be helpful. On Sat, Oct 22, 2016 at 07:36:12AM -0700, GitHub wrote: > Branch: refs/heads/dv-gcc-warning-fixes > Home: https://github.com/fvwmorg/fvwm > Commit: 5b325057dc569621230a2326d1640dcb07cacdb1 > > https://github.com/fvwmorg/fvwm/commit/5b325057dc569621230a2326d1640dcb07cacdb1 > Author: Dominik Vogt> Date: 2016-10-22 (Sat, 22 Oct 2016) > > Changed paths: > M fvwm/add_window.c > M fvwm/events.c > M fvwm/frame.c > M fvwm/icons.c > M fvwm/menus.c > M fvwm/session.c > M libs/FGettext.c > M libs/FImage.c > M libs/FRender.c > M libs/FScreen.c > M libs/Fft.c > M libs/Ficonv.c > M libs/fsm.c > M modules/FvwmConsole/getline.c > > Log Message: > --- > Fix gettext write to read only string; fix warnings. > > * modules/FvwmConsole/getline.c (get_line): Fix warnings. > * fvwm/session.c (set_sm_properties, SessionInit): Fix warnings. > * fvwm/frame.c (frame_prepare_animation_shape) > (frame_setup_shape): Fix warnings. > * fvwm/icons.c (CreateIconWindow): Fix warnings. > * fvwm/add_window.c (setup_style_and_decor): Fix warnings. > * fvwm/events.c (_handle_cr_on_shaped): Fix warnings. > * fvwm/menus.c (size_menu_vertically): Fix write access to read-only > location with gettext. > > libs: > * fsm.c (SaveYourselfPhase2ReqProc, SaveYourselfDoneProc) > (NewConnectionMsg): Fix warnings. > * FGettext.c (FGettextInit): Fix warnings. > * FImage.c (FGetFImage): Fix warnings. > * Fft.c (FftGetFont): Fix warnings. > * Ficonv.c (is_translit_supported, convert_charsets): Fix warnings. > * FRender.c (FRenderCreateShadePicture, FRenderVisualInit) > (FRenderTintRectangle, FRenderTintPicture, FRenderRender): Fix > warnings. > * FScreen.c (FScreenInit): Fix warnings. > > > > Ciao Dominik ^_^ ^_^ -- Dominik Vogt
[fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...
Branch: refs/heads/dv-gcc-warning-fixes Home: https://github.com/fvwmorg/fvwm Commit: 5b325057dc569621230a2326d1640dcb07cacdb1 https://github.com/fvwmorg/fvwm/commit/5b325057dc569621230a2326d1640dcb07cacdb1 Author: Dominik VogtDate: 2016-10-22 (Sat, 22 Oct 2016) Changed paths: M fvwm/add_window.c M fvwm/events.c M fvwm/frame.c M fvwm/icons.c M fvwm/menus.c M fvwm/session.c M libs/FGettext.c M libs/FImage.c M libs/FRender.c M libs/FScreen.c M libs/Fft.c M libs/Ficonv.c M libs/fsm.c M modules/FvwmConsole/getline.c Log Message: --- Fix gettext write to read only string; fix warnings. * modules/FvwmConsole/getline.c (get_line): Fix warnings. * fvwm/session.c (set_sm_properties, SessionInit): Fix warnings. * fvwm/frame.c (frame_prepare_animation_shape) (frame_setup_shape): Fix warnings. * fvwm/icons.c (CreateIconWindow): Fix warnings. * fvwm/add_window.c (setup_style_and_decor): Fix warnings. * fvwm/events.c (_handle_cr_on_shaped): Fix warnings. * fvwm/menus.c (size_menu_vertically): Fix write access to read-only location with gettext. libs: * fsm.c (SaveYourselfPhase2ReqProc, SaveYourselfDoneProc) (NewConnectionMsg): Fix warnings. * FGettext.c (FGettextInit): Fix warnings. * FImage.c (FGetFImage): Fix warnings. * Fft.c (FftGetFont): Fix warnings. * Ficonv.c (is_translit_supported, convert_charsets): Fix warnings. * FRender.c (FRenderCreateShadePicture, FRenderVisualInit) (FRenderTintRectangle, FRenderTintPicture, FRenderRender): Fix warnings. * FScreen.c (FScreenInit): Fix warnings.