Record comparison compiler warning

Started by Bruce Momjianabout 12 years ago7 messages

bruce@momjian.us

about 12 years ago

I am seeing this compiler warning in git head:

rowtypes.c: In function 'record_image_cmp':
rowtypes.c:1433: warning: 'cmpresult' may be used
uninitialized in this function rowtypes.c:1433: note: 'cmpresult' was declared here

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kevin Grittner

kgrittn@ymail.com

about 12 years ago

In reply to: Bruce Momjian (#1)

Re: Record comparison compiler warning

Bruce Momjian <bruce@momjian.us> wrote:

I am seeing this compiler warning in git head:

     rowtypes.c: In function 'record_image_cmp':
     rowtypes.c:1433: warning: 'cmpresult' may be used
     uninitialized in this function rowtypes.c:1433: note: 'cmpresult' was declared here

I had not gotten a warning under either gcc or clang, but that was
probably because I was doing assert-enabled builds, and the
Assert(false) saved me. That seemed a little marginal anyway, so
how about this?:

diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index ae007cf..0332593 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -1508,7 +1508,11 @@ record_image_cmp(FunctionCallInfo fcinfo)
                        break;
 #endif
                    default:
-                       Assert(false);  /* cannot happen */
+                       /* cannot happen */
+                       elog(ERROR,
+                            "unexpected length of %i found comparing columns of type %s",
+                            (int) tupdesc1->attrs[i1]->attlen,
+                            format_type_be(tupdesc1->attrs[i1]->atttypid));
                }
            }
            else

Does that fix the warning for you?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Bruce Momjian

bruce@momjian.us

about 12 years ago

In reply to: Kevin Grittner (#2)

Re: Record comparison compiler warning

On Wed, Oct 16, 2013 at 11:49:13AM -0700, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

I am seeing this compiler warning in git head:

ï¿½ï¿½ï¿½ï¿½ rowtypes.c: In function 'record_image_cmp':
ï¿½ï¿½ï¿½ï¿½ rowtypes.c:1433: warning: 'cmpresult' may be used
ï¿½ï¿½ï¿½ï¿½ uninitialized in this function rowtypes.c:1433: note: 'cmpresult' was declared here

I had not gotten a warning under either gcc or clang, but that was
probably because I was doing assert-enabled builds, and the
Assert(false) saved me.ï¿½ That seemed a little marginal anyway, so
how about this?:

Would you please send the file as ASCII, e.g. not:

<A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0> default:

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ Everyone has their own god. +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kevin Grittner

kgrittn@ymail.com

about 12 years ago

In reply to: Bruce Momjian (#3)

3 attachment(s)

Re: Record comparison compiler warning

Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Oct 16, 2013 at 11:49:13AM -0700, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

I am seeing this compiler warning in git head:

     rowtypes.c: In function 'record_image_cmp':
     rowtypes.c:1433: warning: 'cmpresult' may be used
     uninitialized in this function rowtypes.c:1433: note: 'cmpresult' was declared here

I had not gotten a warning under either gcc or clang, but that was
probably because I was doing assert-enabled builds, and the
Assert(false) saved me. That seemed a little marginal anyway, so
how about this?:

Would you please send the file as ASCII, e.g. not:

<A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0> default:

Huh, I did not see anything remotely like that in my email or in
the archives:

/messages/by-id/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

... however, I'm attaching it rather than pasting it this time. It is warning-recimgcmp.diff.

I'm also attaching the other warnings (outside of the well-known
yyg parser warning) which I get from a compile using Ubuntu clang
version 3.0-6ubuntu3. The one in pg_standby seems completely
legitimate -- what is the point of comparing to see whether an
unsigned integer is >= 0? The other one would be nice to quiet
down, but I would understand if people would prefer to leave it
alone and hope the compiler writers fix it. Warnings those fix
below:

catcache.c:571:12: warning: variable 'cl' is uninitialized when used here [-Wuninitialized]
                                Assert(cl->cl_magic == CL_MAGIC);
                                ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/include/c.h:625:10: note: expanded from:
                Trap(!(condition), "FailedAssertion")
                       ^
../../../../src/include/c.h:607:28: note: expanded from:
                if ((assert_enabled) && (condition)) \
                                         ^~~~~~~~~
catcache.c:569:5: note: variable 'cl' is declared here
                                CatCList   *cl = dlist_container(CatCList, cache_elem, iter.cur);
                                ^
catcache.c:585:13: warning: variable 'ct' is uninitialized when used here [-Wuninitialized]
                                        Assert(ct->ct_magic == CT_MAGIC);
                                        ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~
../../../../src/include/c.h:625:10: note: expanded from:
                Trap(!(condition), "FailedAssertion")
                       ^
../../../../src/include/c.h:607:28: note: expanded from:
                if ((assert_enabled) && (condition)) \
                                         ^~~~~~~~~
catcache.c:583:6: note: variable 'ct' is declared here
                                        CatCTup    *ct = dlist_container(CatCTup, cache_elem, iter.cur);
                                        ^
2 warnings generated.
pg_standby.c:348:22: warning: comparison of unsigned expression >= 0 is always true [-Wtautological-compare]
                if (tli > 0 && log >= 0 && seg > 0)
                               ~~~ ^ ~
1 warning generated.

None of these seem particularly urgent, so I'll leave them for
comment for maybe a week before taking any action.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

warning-recimgcmp.difftext/x-diff; name=warning-recimgcmp.diffDownload

diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index cb04a72..516c37b 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -1508,7 +1508,11 @@ record_image_cmp(FunctionCallInfo fcinfo)
 						break;
 #endif
 					default:
-						Assert(false);	/* cannot happen */
+						/* cannot happen */
+						elog(ERROR,
+							 "unexpected length of %i found comparing columns of type %s",
+							 (int) tupdesc1->attrs[i1]->attlen,
+							 format_type_be(tupdesc1->attrs[i1]->atttypid));
 				}
 			}
 			else

warning-catcache.difftext/x-diff; name=warning-catcache.diffDownload

diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index c467f11..89e6325 100644
--- a/src/backend/utils/cache/catcache.c
+++ b/src/backend/utils/cache/catcache.c
@@ -566,8 +566,9 @@ AtEOXact_CatCache(bool isCommit)
 			/* Check CatCLists */
 			dlist_foreach(iter, &ccp->cc_lists)
 			{
-				CatCList   *cl = dlist_container(CatCList, cache_elem, iter.cur);
+				CatCList   *cl;
 
+				cl = dlist_container(CatCList, cache_elem, iter.cur);
 				Assert(cl->cl_magic == CL_MAGIC);
 				Assert(cl->refcount == 0);
 				Assert(!cl->dead);
@@ -580,8 +581,9 @@ AtEOXact_CatCache(bool isCommit)
 
 				dlist_foreach(iter, bucket)
 				{
-					CatCTup    *ct = dlist_container(CatCTup, cache_elem, iter.cur);
+					CatCTup    *ct;
 
+					ct = dlist_container(CatCTup, cache_elem, iter.cur);
 					Assert(ct->ct_magic == CT_MAGIC);
 					Assert(ct->refcount == 0);
 					Assert(!ct->dead);

warning-standby.difftext/x-diff; name=warning-standby.diffDownload

diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index a3f40fb..059c820 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -345,7 +345,7 @@ SetWALFileNameForCleanup(void)
 	if (keepfiles > 0)
 	{
 		sscanf(nextWALFileName, "%08X%08X%08X", &tli, &log, &seg);
-		if (tli > 0 && log >= 0 && seg > 0)
+		if (tli > 0 && seg > 0)
 		{
 			log_diff = keepfiles / MaxSegmentsPerLogFile;
 			seg_diff = keepfiles % MaxSegmentsPerLogFile;

Stefan Kaltenbrunner

stefan@kaltenbrunner.cc

about 12 years ago

In reply to: Kevin Grittner (#4)

Re: Record comparison compiler warning

On 10/31/2013 07:51 PM, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Oct 16, 2013 at 11:49:13AM -0700, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

I am seeing this compiler warning in git head:

rowtypes.c: In function 'record_image_cmp':
rowtypes.c:1433: warning: 'cmpresult' may be used
uninitialized in this function rowtypes.c:1433: note: 'cmpresult' was declared here

I had not gotten a warning under either gcc or clang, but that was
probably because I was doing assert-enabled builds, and the
Assert(false) saved me. That seemed a little marginal anyway, so
how about this?:

Would you please send the file as ASCII, e.g. not:

<A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0> default:

Huh, I did not see anything remotely like that in my email or in
the archives:

/messages/by-id/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

/messages/by-id/raw/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

Stefan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Alvaro Herrera

alvherre@2ndquadrant.com

about 12 years ago

In reply to: Stefan Kaltenbrunner (#5)

Re: Record comparison compiler warning

Stefan Kaltenbrunner wrote:

On 10/31/2013 07:51 PM, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

On Wed, Oct 16, 2013 at 11:49:13AM -0700, Kevin Grittner wrote:

Bruce Momjian <bruce@momjian.us> wrote:

Would you please send the file as ASCII, e.g. not:

<A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0><A0> default:

Huh, I did not see anything remotely like that in my email or in
the archives:

/messages/by-id/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

/messages/by-id/raw/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

I would blame Bruce's MUA, or surrounding configuration, for this
problem. It looks fine in mine, and as far as I can see, Kevin's
message correctly declares the email to be in Latin-1 quoted-printable
encoding, which declares A0 to mean non-breaking space.

Maybe, for instance, Bruce is running Mutt in Latin-1 mode with the
terminal set to UTF-8 or some such?

--
ï¿½lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 12 years ago

In reply to: Alvaro Herrera (#6)

Re: Record comparison compiler warning

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

Stefan Kaltenbrunner wrote:

/messages/by-id/raw/1381949353.78943.YahooMailNeo@web162902.mail.bf1.yahoo.com

I would blame Bruce's MUA, or surrounding configuration, for this
problem. It looks fine in mine, and as far as I can see, Kevin's
message correctly declares the email to be in Latin-1 quoted-printable
encoding, which declares A0 to mean non-breaking space.

I agree with Bruce: this patch is broken. A0 may be a non-breaking space,
but the fact remains that it isn't a space, and since we're talking about
a diff, the whitespace needs to be the same as what it is in the original
file. (I believe that some of those whitespace runs involve tabs not just
spaces, making the diff even more wrong.)

Admittedly, if you're just eyeballing it, it might look fine. But try
feeding it to "patch" and you'll find out it ain't.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers