Re: Cascaded casts

[Friedrich Beckmann]

Hi John,

yes, I changed the default compiler warning settings with this patch

https://git.savannah.gnu.org/cgit/pspp.git/commit/?id=cc0afe999a50963f6419dd3b9d3f00f50b26c760

The patch applies the AM_CFLAGS settings also for the psppire gui code
but with Wunused-parameter disabled. Before that the
gui code was only compiled with Wall.

AM_CFLAGS enables -Wall and -Wextra as default and disables Wsign-compare (-W 
is the old word for -Wextra)

In addition I disabled the Wunused-parameter for the gui because the gui 
callbacks result in
many warnings - for the core code this is not the case. I also had the 
impression
that with the enabled settings some problems showed up as you mention below.

With the previous patches we are down to these warnings on amd64:

http://caeis.etech.fh-augsburg.de:8010/#/builders/6/builds/41/steps/2/logs/warnings__110_

For i386 there are two more.

Maybe Ben can have a look at the enum casts:

https://git.savannah.gnu.org/cgit/pspp.git/commit/?id=5b2ed095a8aaa98adaea4922855edcbfb619c728

As far as I understood the code this are intentional „extensions“:

https://git.savannah.gnu.org/cgit/pspp.git/tree/src/language/data-io/placement-parser.c?id=5b2ed095a8aaa98adaea4922855edcbfb619c728#n37

An alternative to the function casts for g_list_foreach that I did in the first 
place is to create a new function „free_tree“ with exactly two parameters which 
then calls gtk_tree_path_free. That free_tree function is then used in 
g_list_foreach. But I found that difficult to read due to the additional 
function call level. Therefore I did this cast which relies on this „undefined“ 
compiler behaviour. Would you prefer this alternative?

When I run configure with CFLAGS=„-Wall -Wextra -Wunused-parameter 
-Wsign-compare“ (= full)
then I get the following warnings:

982     sign-compare
620     unused-parameter
18      deprecated-declarations
15      unused-value
6       unused-variable
2       pointer-arith
2       pointer-sign
1       type-limits
1646    Total

This includes the perl module and the gl functions which are both compiled 
without any warning setup per default. With current default compiler setup we 
have

18      deprecated-declarations
2       pointer-arith
2       maybe-uninitialized
1       return-local-addr
23      Total

I think this allows us to spot problematic issues and does not flood us with
warnings that we will never fix.

Fritz

> Am 25.08.2020 um 14:21 schrieb John Darrington <john@darrington.wattle.id.au>:
> 
> Ahh! So you are building using -Wextra
> 
> Most of these warnings do not appear (either with gcc8 or gcc10) when passing
> just -Wall.  I don't think it's realistic to expect the code to be free of
> warnings with -Wextra, and I don't think it's a good practice to add casts
> purely for the purpose of shuting the compiler up.
> 
> The warnings I *do* see with gcc10 -Wall include the enum-conversion warnings 
> in
> data-io/placement-parser.c and output/spv/spv.c  but I don't think casts are 
> the
> right way to fix these.  Rethinking the definition of the enums and their 
> members
> would be a better way IMO, but this is Ben's code so I'll leave that decision 
> to
> him.
> 
> The tooltip related changes are good and I'm glad you caught the switch
> fallthrough issues!  The G_DEFINE_TYPE changes are reasonable too.
> 
> J'
> 
> 
> On Tue, Aug 25, 2020 at 01:16:47PM +0200, Friedrich Beckmann wrote:
>     Hi John,
> 
>     the relevant warning without the change is here:
> 
>     ../pspp/src/ui/gui/psppire-acr.c: In function 
> ???on_change_button_clicked???:
>     ../pspp/src/ui/gui/psppire-acr.c:186:22: warning: cast between 
> incompatible function types from ???void (*)(GtkTreePath *)??? {aka ???void 
> (*)(struct _GtkTreePath *)???} to ???void (*)(void *, void *)??? 
> [-Wcast-function-type]
>        g_list_foreach (l, (GFunc) gtk_tree_path_free, NULL);
> 
>     You can see the full build log here: 
> http://caeis.etech.fh-augsburg.de:8010/#/builders/4/builds/26
> 
>     On debian-buster the compiler is gcc8 while on sid the compiler is gcc10.
> 
>     GFunc is defined with two parameters: 
> https://developer.gnome.org/glib/stable/glib-Doubly-Linked-Lists.html#GFunc
> 
>     while gtk_tree_path_free has only one:
> 
>     
> https://developer.gnome.org/gtk3/stable/GtkTreeModel.html#gtk-tree-path-free
> 
>     I guess it is the same behaviour that resulted in this macro
> 
>     
> https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html#G-SOURCE-FUNC:CAPS
> 
>     A discussion about this check is here:
> 
>     https://gitlab.gnome.org/GNOME/gnome-terminal/-/issues/96
> 
>     resulting in this cast. Root cause seems to be that the behaviour for 
> different number of arguments is undefined.
> 
>     http://c0x.coding-guidelines.com/6.5.2.2.html
> 
>     The clang compiler version 9 does not issue this warning.
> 
>     Fritz
> 
>> Am 25.08.2020 um 07:00 schrieb John Darrington 
>> <john@darrington.wattle.id.au>:
>> 
>> I don't understand this commit, and I think it just makes the code
>> harder to read:
>> 
>> 
>>   commit ceaed4a17cb3b0a14c89f10b72a636f94af97e7a
>>   Author: Friedrich Beckmann <friedrich.beckmann@gmx.de>
>>   Date:   Mon Aug 24 11:19:33 2020 +0200
>> 
>>       Warnings: function type cast for g_list_foreach
>> 
>>       I added a cast for the functions used in g_list_foreach. Without
>>       the cast, the warning
>>       warning: cast between incompatible function types
>>       is reported.
>> 
>>   diff --git a/src/ui/gui/dialog-common.c b/src/ui/gui/dialog-common.c
>>   index f167034b1..1b29c4a3b 100644
>>   --- a/src/ui/gui/dialog-common.c
>>   +++ b/src/ui/gui/dialog-common.c
>>   @@ -112,7 +112,7 @@ homogeneous_types (GtkWidget *source, GtkWidget *dest)
>>          have_type = true;
>>        }
>> 
>>   -  g_list_foreach (list, (GFunc) gtk_tree_path_free, NULL);
>>   +  g_list_foreach (list, (GFunc) (void (*)(void)) gtk_tree_path_free, 
>> NULL);
>> 
>> 
>> gtk_tree_path_free is already cast to GFunc,  so what good does it do to 
>> first
>> cast it to something else, and _then_ to GFunc without using it?   You say 
>> that
>> something is reporting this as a "cast between incompatible types" - That is
>> what casts are for  -  to make incompatible types compatible.
>> 
>> Before, we had:
>> 
>> (void (*) (GtkTreePath *)   -> (void (*) (void *, void *))
>> 
>> Now we have:
>> 
>> (void (*) (GtkTreePath *)  -> (void (*) (void))  -> (void (*) (void *, void 
>> *))
>> 
>> What good does the intermediate cast do?  Which tool is issuing a warning?
>> 
>> J'
>> 
>> 
>