Thread: 2x compile warning
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
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
Kris Jurka <books@ejurka.com> 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
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);
Tom Lane wrote: > Kris Jurka <books@ejurka.com> 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. +
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 <kleptog@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
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
Kris Jurka <books@ejurka.com> 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
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 <kleptog@svana.org> http://svana.org/kleptog/ > From each according to his ability. To each according to his ability to litigate.
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 \ ) \ : \
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
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. +