gcc warnings

[Arun Persaud]

Hi

how about enabling "-Wall -Wno-parentheses" on master and v4.6.x? You
can test the output of this by running

make CFlAGS="-Wall -Wno-parentheses"

ARUN

[Byrial Jensen]

Den 15-04-2012 23:35, Arun Persaud skrev:
> Hi
>
> how about enabling "-Wall -Wno-parentheses" on master and v4.6.x? You
> can test the output of this by running
>
> make CFlAGS="-Wall -Wno-parentheses"
>
> ARUN
>
With the current master, that will give up to 69 warnings to fix:
      25 -Wunused-result (*)
      21 -Wimplicit-function-declaration
       9 -Wunused-but-set-variable
       6 -Wuninitialized (*)
       3 -Wchar-subscripts
       2 -Wunused-function
       1 -Wunused-value
       1 -Wpointer-sign
       1 -Warray-bounds (*)

The warning types marked with (*) is not found when compiled without
optimization.

But no objections from me. I will even volunteer to remove them.

[Arun Persaud]

Hi

> With the current master, that will give up to 69 warnings to fix:
>      25 -Wunused-result (*)
>      21 -Wimplicit-function-declaration
>       9 -Wunused-but-set-variable
>       6 -Wuninitialized (*)
>       3 -Wchar-subscripts
>       2 -Wunused-function
>       1 -Wunused-value
>       1 -Wpointer-sign
>       1 -Warray-bounds (*)
>
> The warning types marked with (*) is not found when compiled without
> optimization.
>
> But no objections from me. I will even volunteer to remove them.

in that case I would say "go for it" :)

Arun

[h.g.muller]

At 14:35 15-4-2012 -0700, Arun Persaud wrote:
>Hi
>
>how about enabling "-Wall -Wno-parentheses" on master and v4.6.x? You
>can test the output of this by running
>
>make CFlAGS="-Wall -Wno-parentheses"
>
>ARUN

That seems a bad idea, because it would just flood us with warnings which
I am dead against fixing, because it wouldmake the code unreadable.
This is a vey counter-productive warning.

If there existed a warning "more than one token on line", would you also
enable it?

[Byrial Jensen]

Den 16-04-2012 09:03, h.g. muller skrev:

> At 14:35 15-4-2012 -0700, Arun Persaud wrote:
>> Hi
>>
>> how about enabling "-Wall -Wno-parentheses" on master and v4.6.x? You
>> can test the output of this by running
>>
>> make CFlAGS="-Wall -Wno-parentheses"
>>
>> ARUN
>
> That seems a bad idea, because it would just flood us with warnings which
> I am dead against fixing, because it wouldmake the code unreadable.

I just removed the 9 warnings of type unused-but-set-variable and the 2
warnings of type unused-function that would be enabled by Arun's
proposal in the master branch. I totally cannot see how these removals
make the code more unreadable.

[Byrial Jensen]

Den 16-04-2012 13:30, Byrial Jensen skrev:

> Den 16-04-2012 09:03, h.g. muller skrev:
>> At 14:35 15-4-2012 -0700, Arun Persaud wrote:
>>> Hi
>>>
>>> how about enabling "-Wall -Wno-parentheses" on master and v4.6.x? You
>>> can test the output of this by running
>>>
>>> make CFlAGS="-Wall -Wno-parentheses"
>>>
>>> ARUN
>>
>> That seems a bad idea, because it would just flood us with warnings
>> which
>> I am dead against fixing, because it wouldmake the code unreadable.
>
> I just removed the 9 warnings of type unused-but-set-variable and the
> 2 warnings of type unused-function that would be enabled by Arun's
> proposal in the master branch. I totally cannot see how these removals
> make the code more unreadable.

PS. It may be a good idea to check if the removals of unused variables
and functions are correct. Maybe some the variables or functions should
have been used for correct behavior of the program. If that was the
case, the warnings were indeed useful to discover the error.

[Byrial Jensen]

Den 16-04-2012 15:17, h.g. muller skrev:
>
>>> I just removed the 9 warnings of type unused-but-set-variable and
>>> the 2 warnings of type unused-function that would be enabled by
>>> Arun's proposal in the master branch. I totally cannot see how these
>>> removals make the code more unreadable.
>
> Perhaps that is because they have nothing to do with the
> 'no-parentheses' warnings?

"-Wno-parentheses" means that warnings suggesting parentheses which were
otherwise enabled by -Wall are turned off. So what Arun suggested was to
enable the suite of warnings enabled by -Wall except for warnings about
parentheses.

Generally -Wno-somethning will turn off warnings of type -Wsomething.

[h.g.muller]

>>I just removed the 9 warnings of type unused-but-set-variable and the 2
>>warnings of type unused-function that would be enabled by Arun's proposal
>>in the master branch. I totally cannot see how these removals make the
>>code more unreadable.

Perhaps that is because they have nothing to do with the 'no-parentheses'
warnings?

>PS. It may be a good idea to check if the removals of unused variables and
>functions are correct. Maybe some the variables or functions should have
>been used for correct behavior of the program. If that was the case, the
>warnings were indeed useful to discover the error.

Sure, I will check whatever was changed, to see if removal was the proper
fix, or if the warning was caused by a deeper problem.

The really nasty problem, however, is not so much the warning of implicit
function declarations, but of functions that have prototypes in separate .c
files, rather than in a common .h file. And similarly, variables that are
declared external in a .c file, without occurring in a .h file. I tried to
fix that wherever I encountered it during the refactoring of master. (But
that did not progress any further than the X11 front-end yet.)

It is also highly recommendable that all variables shared between front-end
and back-end allocate space in the back-end. So that new front-ends that
don't need them do not run into undefined reference errors. It would be
still better if such variables were not needed at all, and all info was
passed as function arguments.

[Byrial Jensen]

Den 16-04-2012 15:43, h.g. muller skrev:

> I am absolutely fine with the other warnings. Although I sometimes
> encounter an annoying one about library functions declared with
> 'warn-unused-result' (or something like that). If I am not interested
> in the result, I don't think the code would improve very much by
> fooling it into thinking I am. And not being interested in the result
> is often a sign of laziness or it not being clear beforehand how to
> solve possible error conditions. In that case patching it up by
> casting to (void) or some other trick only serves to hide permananetly
> what could be a genuine problem, so that you now know for sure it will
> be never fixed.

Warnings like:

backend.c:12321:2: advarsel: ignoring return value of ‘fgets’, declared
with attribute warn_unused_result [-Wunused-result]

wont be turned off by casting to void. (just see in backend.c:12321).
Beside fgets() also calls of pipe(), system(), getcwd(), chdir(), nice()
give this warning. Either the return codes should be really checked or
the warning can be turned off with "-Wno-unused-result".

What IMO should be checked and fixed is the "may be used uninitialized"
warnings:

backend.c: In function 'LeftClick':
backend.c:7306:8: warning: 'saveAnimate' may be used uninitialized in
this function [-Wuninitialized]

book.c: In function 'entry_from_file':
book.c:391:19: warning: 'r' may be used uninitialized in this function
[-Wuninitialized]
book.c:400:12: note: 'r' was declared here

menus.c: In function 'MenuNameToItem':
menus.c:886:50: warning: 'i' may be used uninitialized in this function
[-Wuninitialized]

xoptions.c: In function 'GenericPopUp':
xoptions.c:994:62: warning: 'box' may be used uninitialized in this
function [-Wuninitialized]
xoptions.c:897:8: warning: 'forelast' may be used uninitialized in this
function [-Wuninitialized]
xoptions.c:995:46: warning: 'oldForm' may be used uninitialized in this
function [-Wuninitialized]

[h.g.muller]

At 14:23 16-4-2012 +0200, Byrial Jensen wrote:
"-Wno-parentheses" means that warnings suggesting parentheses which were
otherwise enabled by -Wall are turned off. So what Arun suggested was to
enable the suite of warnings enabled by -Wall except for warnings about
parentheses.

>Generally -Wno-somethning will turn off warnings of type -Wsomething.

Oh sorry, then I misunderstood that completely. I thought the warning would
warn if there were no parentheses.
I am absolutely fine with the other warnings. Although I sometimes
encounter an annoying one about library functions declared with
'warn-unused-result' (or something like that). If I am not interested in
the result, I don't think the code would improve very much by fooling it
into thinking I am. And not being interested in the result is often a sign
of laziness or it not being clear beforehand how to solve possible error
conditions. In that case patching it up by casting to (void) or some other
trick only serves to hide permananetly what could be a genuine problem, so
that you now know for sure it will be never fixed.

So I am all for deleting unused variables, whose presence is an errror or
legacy of now rewritten code. Deleting unused functions IMO is already of
more doubtful use, if the finctions are part of a generally useful
infra-structure. But silencing errors that have a vague ring of truth to
them by a meaningless trick is probably something that is better to refrain
from.

[h.g.muller]

> What IMO should be checked and fixed is the "may be used uninitialized"
> warnings:
>
> backend.c: In function 'LeftClick':
> backend.c:7306:8: warning: 'saveAnimate' may be used uninitialized in
> this function [-Wuninitialized]
>
> menus.c: In function 'MenuNameToItem':
> menus.c:886:50: warning: 'i' may be used uninitialized in this function
> [-Wuninitialized]

These two seem to be genuine bugs. The first I just introduced, and it
seems I only got away with it because the formally un-initialized memory
location still holds the value from the previous call to LeftClick, on the
down-click. (I was under the impression that this was a static variable,
but it is local!)

For i to be used uninitialized in menus.c, one would have to feed
MenuNameToItem a menu name without a period in it, which currently does
not happen in the code, but could happen if a user defines a key binding
in .Xresources with such an item name, so we'd better make it resistant to
that.

I will take care of these two when you are done with all other warnings.

> book.c: In function 'entry_from_file':
> book.c:391:19: warning: 'r' may be used uninitialized in this function
> [-Wuninitialized]
> book.c:400:12: note: 'r' was declared here
>
> xoptions.c: In function 'GenericPopUp':
> xoptions.c:994:62: warning: 'box' may be used uninitialized in this
> function [-Wuninitialized] xoptions.c:897:8: warning: 'forelast' may be
> used uninitialized in this function [-Wuninitialized] xoptions.c:995:46:
> warning: 'oldForm' may be used uninitialized in this
> function [-Wuninitialized]

The one in book.c is a red herring: a pointer to the variable is passed to
a routine that initializes it, but the compiler cannot see that. The
warning can be safely suppressed by just declaring as r=0. I guess the
compiler is formally right in that the GenericPopUp is not fool-proof; the
code relies on BoxEnd options in the Option list that describes a dialog
will not occur before a BoxBegin, and SAME_ROW options not being used as
first Option in a list. Since we only feed it lists defined in our own
code, we can guarantee this. So I think that here we should just silence
it too by assignment of NULL to all these variables in their declaration.
The routine is basically an interpreter to handle the dialog descriptions;
making it check the correctness of the discriptions seems like overdoing
it. Option lists from engines will never contain BoxBegin, BoxEnd or
SAME_ROW elements.

[Byrial Jensen]

Den 16-04-2012 19:18, h.g.muller@... skrev:

>  book.c: In function 'entry_from_file':
>  book.c:391:19: warning: 'r' may be used uninitialized in this function [-Wuninitialized]
>  book.c:400:12: note: 'r' was declared here



 > The one in book.c is a red herring: a pointer to the variable is
passed to a routine that initializes it,
 > but the compiler cannot see that. The warning can be safely
suppressed by just declaring as r=0.

It sure looks like a genuine bug to me. The variable r is declared in
function entry_from_file() line 400, and is never initialized before it
is used in function int_from_file() line 391. Just like gcc says - it
indeed does some interprocedural analysis.

[Michel Van den Bergh]

Yep it does like a bug. I am afraid I am to blame for this....
It is remarkable that this never seems to have given any issues.

Michel


On 04/16/2012 09:24 PM, Byrial Jensen wrote:

> Den 16-04-2012 19:18, h.g.muller@... skrev:
>
>>   book.c: In function 'entry_from_file':
>>   book.c:391:19: warning: 'r' may be used uninitialized in this function [-Wuninitialized]
>>   book.c:400:12: note: 'r' was declared here
>
>
>   >  The one in book.c is a red herring: a pointer to the variable is
> passed to a routine that initializes it,
>   >  but the compiler cannot see that. The warning can be safely
> suppressed by just declaring as r=0.
>
> It sure looks like a genuine bug to me. The variable r is declared in
> function entry_from_file() line 400, and is never initialized before it
> is used in function int_from_file() line 391. Just like gcc says - it
> indeed does some interprocedural analysis.
>
>

[h.g.muller]

Wow, changing the default flags sure caused a lot of new warnings.
A lot of the implicit declarations were indeed pointing out code deficiencies
(prototype in a non-included header, or no prototype at all). I fixed all
of these.
In some cases it was best to actually move the routine to another file.

I fixed the warnings in MenuNameToItem and for saveAnimate.

I also had a look to the warnings about signedness and char used as index,
and I think these are appropriately solved now too. If I now do a standard
'make', it is completely free of warnings again.

[Arun Persaud]

On 04/17/2012 08:02 AM, h.g. muller wrote:

> Wow, changing the default flags sure caused a lot of new warnings.
> A lot of the implicit declarations were indeed pointing out code
> deficiencies
> (prototype in a non-included header, or no prototype at all). I fixed
> all of these.
> In some cases it was best to actually move the routine to another file.
>
> I fixed the warnings in MenuNameToItem and for saveAnimate.
>
> I also had a look to the warnings about signedness and char used as index,
> and I think these are appropriately solved now too. If I now do a standard
> 'make', it is completely free of warnings again.

anything in there worth porting to the v4.6.x branch? In that case we
could include it in 4.6.2

ARUN

[h.g.muller]

> anything in there worth porting to the v4.6.x branch? In that case we
> could include it in 4.6.2

No, I don't think so. The only things that were real errors were in new
code, and most of the implicit declarations were caused by splitting up
files so that the callers got separated from the functions they called.

I did find a new buglet w.r.t. casting rights when editing positions in
variants with holdings, though. (Q-side rights were never granted, not
even if there was a Rook on a1/a8.) I already pushed the fix to Savannah
v4.6.x (plus another cosmetic one).