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
Attachments:
heapam-no-isnull-check.patchtext/plain; charset=US-ASCII; name=heapam-no-isnull-check.patchDownload
Index: src/include/access/heapam.h
===================================================================
RCS file: /projects/cvsroot/pgsql/src/include/access/heapam.h,v
retrieving revision 1.107
diff -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 18:12:16 -0000
***************
*** 45,51 ****
#define fastgetattr(tup, attnum, tupleDesc, isnull) \
( \
AssertMacro((attnum) > 0), \
! ((isnull) ? (*(isnull) = false) : (dummyret)NULL), \
HeapTupleNoNulls(tup) ? \
( \
(tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \
--- 45,51 ----
#define fastgetattr(tup, attnum, tupleDesc, isnull) \
( \
AssertMacro((attnum) > 0), \
! (*(isnull) = false), \
HeapTupleNoNulls(tup) ? \
( \
(tupleDesc)->attrs[(attnum)-1]->attcacheoff >= 0 ? \
***************
*** 61,67 ****
( \
att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
( \
! ((isnull) ? (*(isnull) = true) : (dummyret)NULL), \
(Datum)NULL \
) \
: \
--- 61,67 ----
( \
att_isnull((attnum)-1, (tup)->t_data->t_bits) ? \
( \
! (*(isnull) = true), \
(Datum)NULL \
) \
: \
***************
*** 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) = true), \
(Datum)NULL \
) \
: \
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. +
Attachments:
/bjm/difftext/plainDownload
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/
Show quoted text
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/
Show quoted text
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. +
Attachments:
/bjm/difftext/plainDownload
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. +