2x compile warning

От:

Gevik Babakhani <pgdev@xs4all.nl>

Дата:

24 апреля 2006 г. в 14:41:33

I noticed the following compile warnings. Perhaps someone is interested
to know about them.

/usr/bin/flex  -o'pgc.c' pgc.l
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g
-Wno-error  -I./../include -I. -I../../../../src/include -D_GNU_SOURCE
-DMAJOR_VERSION=4 -DMINOR_VERSION=2 -DPATCHLEVEL=1  -c -o preproc.o
preproc.c -MMD
In file included from preproc.y:6606:
pgc.c: In function ‘yylex’:
pgc.c:1549: warning: label ‘find_rule’ defined but not used

_GNU_SOURCE   -c -o mac.o mac.c -MMD
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline
-Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g
-I../../../../src/include -D_GNU_SOURCE   -c -o inet_net_ntop.o
inet_net_ntop.c -MMD
inet_net_ntop.c: In function ‘inet_cidr_ntop_ipv6’:
inet_net_ntop.c:292: warning: value computed is not used

Re: 2x compile warning

От:

Bruce Momjian <pgman@candle.pha.pa.us>

Дата:

24 апреля 2006 г. в 17:08:59

Gevik Babakhani wrote:
> I noticed the following compile warnings. Perhaps someone is interested
> to know about them.
> 
> /usr/bin/flex  -o'pgc.c' pgc.l
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline
> -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g
> -Wno-error  -I./../include -I. -I../../../../src/include -D_GNU_SOURCE
> -DMAJOR_VERSION=4 -DMINOR_VERSION=2 -DPATCHLEVEL=1  -c -o preproc.o
> preproc.c -MMD
> In file included from preproc.y:6606:
> pgc.c: In function ?yylex?:
> pgc.c:1549: warning: label ?find_rule? defined but not used

This is a standard warning generated by bison and we can't get rid of
it.

> _GNU_SOURCE   -c -o mac.o mac.c -MMD
> gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Winline
> -Wdeclaration-after-statement -Wendif-labels -fno-strict-aliasing -g
> -I../../../../src/include -D_GNU_SOURCE   -c -o inet_net_ntop.o
> inet_net_ntop.c -MMD
> inet_net_ntop.c: In function ?inet_cidr_ntop_ipv6?:
> inet_net_ntop.c:292: warning: value computed is not used

I have fixed this by adding a (void) cast, patch attached.

-- 
  Bruce Momjian   http://candle.pha.pa.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/backend/utils/adt/inet_net_ntop.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/inet_net_ntop.c,v
retrieving revision 1.21
diff -c -c -r1.21 inet_net_ntop.c
*** src/backend/utils/adt/inet_net_ntop.c	15 Oct 2005 02:49:28 -0000	1.21
--- src/backend/utils/adt/inet_net_ntop.c	24 Apr 2006 19:49:04 -0000
***************
*** 289,295 ****
  		}
  	}
  	/* Format CIDR /width. */
! 	SPRINTF((cp, "/%u", bits));
  	if (strlen(outbuf) + 1 > size)
  		goto emsgsize;
  	strcpy(dst, outbuf);
--- 289,295 ----
  		}
  	}
  	/* Format CIDR /width. */
! 	(void) SPRINTF((cp, "/%u", bits));
  	if (strlen(outbuf) + 1 > size)
  		goto emsgsize;
  	strcpy(dst, outbuf);

Re: 2x compile warning

От:

Bruce Momjian <pgman@candle.pha.pa.us>

Дата:

24 апреля 2006 г. в 17:17:44

Tom Lane wrote:
> Kris Jurka  writes:
> > Also I was testing a gcc 4.2 snapshot (20060419) and it has a whole lot of 
> > warnings stemming from heap_getattr's isnull check:
> > aclchk.c:791: warning: the address of 'isNull', will always evaluate as 
> > 'true'
> 
> We need to lobby the gcc maintainers to not give warnings about valid
> and perfectly reasonable code.  I'm not thrilled with changing the API
> of a key macro for no other reason than that gcc has started to complain
> about it.

Right.  The issue is that when I originally transfered that function to
a macro, the isnull parameter was optionally 0/NULL. While our code
doesn't use that ability, I see no reason to remove it.

I understand why it is complaining because you are really doing if
(&var), but it is a macro, so it can be used in other circumstances as
well.

--  Bruce Momjian   http://candle.pha.pa.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Re: 2x compile warning

От:

Martijn van Oosterhout <kleptog@svana.org>

Дата:

24 апреля 2006 г. в 17:55:36

On Mon, Apr 24, 2006 at 01:16:04PM -0500, Kris Jurka wrote:
> 
> 
> On Mon, 24 Apr 2006, Gevik Babakhani wrote:
> 
> >I noticed the following compile warnings. Perhaps someone is interested
> >to know about them.
> 
> Also I was testing a gcc 4.2 snapshot (20060419) and it has a whole lot of 
> warnings stemming from heap_getattr's isnull check:
> 
> aclchk.c:791: warning: the address of 'isNull', will always evaluate as 
> 'true'

Perhaps someone could check if changing the test explicitly check
against NULL:

>             ((attnum) > (int) (tup)->t_data->t_natts) ? \
>             ( \
>                 (((isnull) != NULL)? (*(isnull) = true) : (dummyret)NULL), \
>                 (Datum)NULL \

removes the warning. It seems silly for the GCC people to add warnings
for this kind of stuff without a simple way to bypass it...
-- 
Martijn van Oosterhout      http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: 2x compile warning

От:

Martijn van Oosterhout <kleptog@svana.org>

Дата:

24 апреля 2006 г. в 18:57:21

On Mon, Apr 24, 2006 at 05:39:30PM -0400, Tom Lane wrote:
> > Yes, this coding removes the warning.
> 
> Oh, good, that seems like a reasonable change to make (it's arguably
> more clear than the original anyway).
> 
> Is this the only place where the warning shows up?  ISTM there's quite
> a lot of code that uses "if (ptr)" for a NULL-ness check.

But very little code of the form "if (&local_var)" which is always true
and what causes the problem here. I think this is just a variation on
the compiler test for "if (i=1)" which is also always true but probably
a bug. That warning you avoid with an extra set of parenthesis.

I'd be surprised if there were many other issues here, only complex
macros are likely to cause this one.

Have a nice day,
-- 
Martijn van Oosterhout      http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: 2x compile warning

От:

Bruce Momjian <pgman@candle.pha.pa.us>

Дата:

24 апреля 2006 г. в 19:05:21

Kris Jurka wrote:
> 
> 
> On Mon, 24 Apr 2006, Martijn van Oosterhout wrote:
> 
> > Perhaps someone could check if changing the test explicitly check
> > against NULL:
> >
> >>             ((attnum) > (int) (tup)->t_data->t_natts) ? \
> >>             ( \
> >>                 (((isnull) != NULL)? (*(isnull) = true) : (dummyret)NULL), \
> >>                 (Datum)NULL \
> >
> > removes the warning. It seems silly for the GCC people to add warnings
> > for this kind of stuff without a simple way to bypass it...
> 
> Yes, this coding removes the warning.

Great, fix attached and applied.

-- 
  Bruce Momjian   http://candle.pha.pa.us
  EnterpriseDB    http://www.enterprisedb.com

  + If your life is a hard drive, Christ can be your backup. +
Index: src/include/access/heapam.h
===================================================================
RCS file: /cvsroot/pgsql/src/include/access/heapam.h,v
retrieving revision 1.107
diff -c -c -r1.107 heapam.h
*** src/include/access/heapam.h	24 Mar 2006 04:32:13 -0000	1.107
--- src/include/access/heapam.h	24 Apr 2006 22:03:14 -0000
***************
*** 100,106 ****
  		( \
  			((attnum) > (int) (tup)->t_data->t_natts) ? \
  			( \
! 				((isnull) ? (*(isnull) = true) : (dummyret)NULL), \
  				(Datum)NULL \
  			) \
  			: \
--- 100,106 ----
  		( \
  			((attnum) > (int) (tup)->t_data->t_natts) ? \
  			( \
! 				((isnull) != NULL) ? (*(isnull) = true) : (dummyret)NULL), \
  				(Datum)NULL \
  			) \
  			: \

Re: 2x compile warning

От:

Bruce Momjian <pgman@candle.pha.pa.us>

Дата:

24 апреля 2006 г. в 19:22:11

Kris Jurka wrote:
> 
> 
> On Mon, 24 Apr 2006, Bruce Momjian wrote:
> 
> > Great, fix attached and applied.
> >
> 
> You also need to change lines 48 and 64 of heapam.h to use the same 
> coding.

Done.

--  Bruce Momjian   http://candle.pha.pa.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

Re: 2x compile warning

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

24 апреля 2006 г. в 17:07:29

Kris Jurka  writes:
> Also I was testing a gcc 4.2 snapshot (20060419) and it has a whole lot of 
> warnings stemming from heap_getattr's isnull check:
> aclchk.c:791: warning: the address of 'isNull', will always evaluate as 
> 'true'

We need to lobby the gcc maintainers to not give warnings about valid
and perfectly reasonable code.  I'm not thrilled with changing the API
of a key macro for no other reason than that gcc has started to complain
about it.
		regards, tom lane

Re: 2x compile warning

От:

Tom Lane <tgl@sss.pgh.pa.us>

Дата:

24 апреля 2006 г. в 18:39:46

Kris Jurka  writes:
> On Mon, 24 Apr 2006, Martijn van Oosterhout wrote:
>> Perhaps someone could check if changing the test explicitly check
>> against NULL:
>> 
>> ((attnum) > (int) (tup)->t_data->t_natts) ? \
>> ( \
>> (((isnull) != NULL)? (*(isnull) = true) : (dummyret)NULL), \
>> (Datum)NULL \
>> 
>> removes the warning. It seems silly for the GCC people to add warnings
>> for this kind of stuff without a simple way to bypass it...

> Yes, this coding removes the warning.

Oh, good, that seems like a reasonable change to make (it's arguably
more clear than the original anyway).

Is this the only place where the warning shows up?  ISTM there's quite
a lot of code that uses "if (ptr)" for a NULL-ness check.
		regards, tom lane

Re: 2x compile warning

От:

Kris Jurka <books@ejurka.com>

Дата:

24 апреля 2006 г. в 16:06:20

On Mon, 24 Apr 2006, Gevik Babakhani wrote:

> I noticed the following compile warnings. Perhaps someone is interested
> to know about them.

Also I was testing a gcc 4.2 snapshot (20060419) and it has a whole lot of 
warnings stemming from heap_getattr's isnull check:

aclchk.c:791: warning: the address of 'isNull', will always evaluate as 
'true'

aclDatum = heap_getattr(tuple, Anum_pg_database_datacl, 
RelationGetDescr(relation), &isNull);

#define heap_getattr(tup, attnum, tupleDesc, isnull) \
( \    AssertMacro((tup) != NULL), \    ( \        ((attnum) > 0) ? \        ( \            ((attnum) > (int) (tup)->t_data->t_natts) ? \            ( \                ((isnull) ? (*(isnull) = true) : (dummyret)NULL), \                (Datum)NULL \            ) \            : \                fastgetattr((tup), (attnum), (tupleDesc), (isnull)) \        ) \        : \            heap_getsysattr((tup), (attnum), (tupleDesc), (isnull)) \    ) \
)

Removing the check for (isnull) before (*(isnull) = true) as in the 
attached patch passes make check, but I have not looked at every 
heap_getattr call site to ensure it's passing a valid isnull pointer.

Kris Jurka

Re: 2x compile warning

От:

Kris Jurka <books@ejurka.com>

Дата:

24 апреля 2006 г. в 18:07:14



On Mon, 24 Apr 2006, Martijn van Oosterhout wrote:

> Perhaps someone could check if changing the test explicitly check
> against NULL:
>
>>             ((attnum) > (int) (tup)->t_data->t_natts) ? \
>>             ( \
>>                 (((isnull) != NULL)? (*(isnull) = true) : (dummyret)NULL), \
>>                 (Datum)NULL \
>
> removes the warning. It seems silly for the GCC people to add warnings
> for this kind of stuff without a simple way to bypass it...

Yes, this coding removes the warning.

Kris Jurka

Re: 2x compile warning

От:

Kris Jurka <books@ejurka.com>

Дата:

24 апреля 2006 г. в 19:12:46



On Mon, 24 Apr 2006, Bruce Momjian wrote:

> Great, fix attached and applied.
>

You also need to change lines 48 and 64 of heapam.h to use the same 
coding.

Kris Jurka