Re: [fvwmorg/fvwm] 5b3250: Fix gettext write to read only string; fix warning...

[Dominik Vogt]

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

[Florian Schmidt]

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

[Florian Schmidt]

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

[Dominik Vogt]

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

[Thomas Adam]

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

[Dominik Vogt]

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

[Thomas Adam]

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

[Dominik Vogt]

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

[GitHub]

  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.