Fix most -Wundef warnings
During the cleanup of the _MSC_VER versions (commit
38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
-Wundef, but that resulted in a bunch of gratuitous warnings. Here is a
patch to fix those. Most of these are just stylistic cleanups, but the
change in pg_bswap.h is potentially useful to avoid misuse by
third-party extensions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Fix-most-Wundef-warnings.patchtext/plain; charset=UTF-8; name=0001-Fix-most-Wundef-warnings.patch; x-mac-creator=0; x-mac-type=0Download
From 86e5e00e17762c174624f203b9c54fdf5fe815fb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 23 Sep 2019 22:50:05 +0200
Subject: [PATCH] Fix most -Wundef warnings
In some cases #if was used instead of #ifdef in an inconsistent style.
Cleaning this up also helps when analyzing cases like
38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd where this makes a
difference.
There are no behavior changes here, but the change in pg_bswap.h would
prevent possible accidental misuse by third-party code.
---
contrib/hstore/hstore_compat.c | 2 +-
contrib/pg_standby/pg_standby.c | 2 +-
contrib/pgcrypto/imath.c | 4 ++--
src/backend/libpq/be-fsstubs.c | 4 ++--
src/backend/replication/logical/reorderbuffer.c | 2 +-
src/backend/storage/file/fd.c | 2 +-
src/backend/utils/hash/dynahash.c | 14 +++++++-------
src/backend/utils/mmgr/freepage.c | 2 +-
src/include/port/pg_bswap.h | 8 ++++++++
src/port/snprintf.c | 4 ++--
src/port/win32error.c | 2 +-
11 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/contrib/hstore/hstore_compat.c b/contrib/hstore/hstore_compat.c
index 1d4e7484e4..d75e9cb23f 100644
--- a/contrib/hstore/hstore_compat.c
+++ b/contrib/hstore/hstore_compat.c
@@ -299,7 +299,7 @@ hstoreUpgrade(Datum orig)
if (valid_new)
{
-#if HSTORE_IS_HSTORE_NEW
+#ifdef HSTORE_IS_HSTORE_NEW
elog(WARNING, "ambiguous hstore value resolved as hstore-new");
/*
diff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index 031b1b5cd5..9fae146dd6 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -145,7 +145,7 @@ CustomizableInitialize(void)
switch (restoreCommandType)
{
case RESTORE_COMMAND_LINK:
-#if HAVE_WORKING_LINK
+#ifdef HAVE_WORKING_LINK
SET_RESTORE_COMMAND("ln -s -f", WALFilePath, xlogFilePath);
break;
#endif
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index 6936d2cdca..545f148a76 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -2361,12 +2361,12 @@ s_ucmp(mp_int a, mp_int b)
static int
s_vcmp(mp_int a, mp_small v)
{
-#if _MSC_VER
+#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4146)
#endif
mp_usmall uv = (v < 0) ? -(mp_usmall) v : (mp_usmall) v;
-#if _MSC_VER
+#ifdef _MSC_VER
#pragma warning(pop)
#endif
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index a750f921fa..b0ece7ec25 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -95,7 +95,7 @@ be_lo_open(PG_FUNCTION_ARGS)
LargeObjectDesc *lobjDesc;
int fd;
-#if FSDB
+#ifdef FSDB
elog(DEBUG4, "lo_open(%u,%d)", lobjId, mode);
#endif
@@ -118,7 +118,7 @@ be_lo_close(PG_FUNCTION_ARGS)
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("invalid large-object descriptor: %d", fd)));
-#if FSDB
+#ifdef FSDB
elog(DEBUG4, "lo_close(%d)", fd);
#endif
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 8ce28ad629..62e54240ec 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3247,7 +3247,7 @@ typedef struct RewriteMappingFile
char fname[MAXPGPATH];
} RewriteMappingFile;
-#if NOT_USED
+#ifdef NOT_USED
static void
DisplayMapping(HTAB *tuplecid_data)
{
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 94be62fa6e..fe2bb8f859 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -738,7 +738,7 @@ durable_link_or_rename(const char *oldfile, const char *newfile, int elevel)
if (fsync_fname_ext(oldfile, false, false, elevel) != 0)
return -1;
-#if HAVE_WORKING_LINK
+#ifdef HAVE_WORKING_LINK
if (link(oldfile, newfile) < 0)
{
ereport(elevel,
diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 0dfbec8e3e..de16adef17 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -243,7 +243,7 @@ struct HTAB
*/
#define MOD(x,y) ((x) & ((y)-1))
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
static long hash_accesses,
hash_collisions,
hash_expansions;
@@ -706,7 +706,7 @@ init_htab(HTAB *hashp, long nelem)
/* Choose number of entries to allocate at a time */
hctl->nelem_alloc = choose_nelem_alloc(hctl->entrysize);
-#if HASH_DEBUG
+#ifdef HASH_DEBUG
fprintf(stderr, "init_htab:\n%s%p\n%s%ld\n%s%ld\n%s%d\n%s%ld\n%s%u\n%s%x\n%s%x\n%s%ld\n",
"TABLE POINTER ", hashp,
"DIRECTORY SIZE ", hctl->dsize,
@@ -832,7 +832,7 @@ hash_destroy(HTAB *hashp)
void
hash_stats(const char *where, HTAB *hashp)
{
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
fprintf(stderr, "%s: this HTAB -- accesses %ld collisions %ld\n",
where, hashp->hctl->accesses, hashp->hctl->collisions);
@@ -933,7 +933,7 @@ hash_search_with_hash_value(HTAB *hashp,
HASHBUCKET *prevBucketPtr;
HashCompareFunc match;
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
hash_accesses++;
hctl->accesses++;
#endif
@@ -988,7 +988,7 @@ hash_search_with_hash_value(HTAB *hashp,
break;
prevBucketPtr = &(currBucket->link);
currBucket = *prevBucketPtr;
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
hash_collisions++;
hctl->collisions++;
#endif
@@ -1130,7 +1130,7 @@ hash_update_hash_key(HTAB *hashp,
HASHBUCKET *oldPrevPtr;
HashCompareFunc match;
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
hash_accesses++;
hctl->accesses++;
#endif
@@ -1204,7 +1204,7 @@ hash_update_hash_key(HTAB *hashp,
break;
prevBucketPtr = &(currBucket->link);
currBucket = *prevBucketPtr;
-#if HASH_STATISTICS
+#ifdef HASH_STATISTICS
hash_collisions++;
hctl->collisions++;
#endif
diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/freepage.c
index ba3bc20ef1..faf5d10158 100644
--- a/src/backend/utils/mmgr/freepage.c
+++ b/src/backend/utils/mmgr/freepage.c
@@ -164,7 +164,7 @@ static void FreePagePushSpanLeader(FreePageManager *fpm, Size first_page,
static Size FreePageManagerLargestContiguous(FreePageManager *fpm);
static void FreePageManagerUpdateLargest(FreePageManager *fpm);
-#if FPM_EXTRA_ASSERTS
+#ifdef FPM_EXTRA_ASSERTS
static Size sum_free_pages(FreePageManager *fpm);
#endif
diff --git a/src/include/port/pg_bswap.h b/src/include/port/pg_bswap.h
index dbf9df6155..dcd3eb08db 100644
--- a/src/include/port/pg_bswap.h
+++ b/src/include/port/pg_bswap.h
@@ -139,7 +139,14 @@ pg_bswap64(uint64 x)
* the same result as a memcmp() of the corresponding original Datums, but can
* be much cheaper. It's generally safe to do this on big-endian systems
* without any special transformation occurring first.
+ *
+ * If SIZEOF_DATUM is not defined, then postgres.h wasn't included and these
+ * macros probably shouldn't be used, so we define nothing. Note that things
+ * like SIZEOF_DATUM == 8 would evaluate as 0 == 8 that case, so this could
+ * lead to the wrong implementation being selected and confusing errors, so
+ * defining nothing is safest.
*/
+#ifdef SIZEOF_DATUM
#ifdef WORDS_BIGENDIAN
#define DatumBigEndianToNative(x) (x)
#else /* !WORDS_BIGENDIAN */
@@ -149,5 +156,6 @@ pg_bswap64(uint64 x)
#define DatumBigEndianToNative(x) pg_bswap32(x)
#endif /* SIZEOF_DATUM == 8 */
#endif /* WORDS_BIGENDIAN */
+#endif /* SIZEOF_DATUM */
#endif /* PG_BSWAP_H */
diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index 8fd997553e..e4bb15dfec 100644
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -1052,7 +1052,7 @@ fmtint(long long value, char type, int forcesign, int leftjust,
}
/* disable MSVC warning about applying unary minus to an unsigned value */
-#if _MSC_VER
+#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable: 4146)
#endif
@@ -1061,7 +1061,7 @@ fmtint(long long value, char type, int forcesign, int leftjust,
uvalue = -(unsigned long long) value;
else
uvalue = (unsigned long long) value;
-#if _MSC_VER
+#ifdef _MSC_VER
#pragma warning(pop)
#endif
diff --git a/src/port/win32error.c b/src/port/win32error.c
index 7fd29eab37..f0582c8dad 100644
--- a/src/port/win32error.c
+++ b/src/port/win32error.c
@@ -188,7 +188,7 @@ _dosmaperr(unsigned long e)
ereport(DEBUG5,
(errmsg_internal("mapped win32 error code %lu to %d",
e, doserr)));
-#elif FRONTEND_DEBUG
+#elif defined(FRONTEND_DEBUG)
fprintf(stderr, "mapped win32 error code %lu to %d", e, doserr);
#endif
errno = doserr;
--
2.23.0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
During the cleanup of the _MSC_VER versions (commit
38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
-Wundef, but that resulted in a bunch of gratuitous warnings. Here is a
patch to fix those. Most of these are just stylistic cleanups, but the
change in pg_bswap.h is potentially useful to avoid misuse by
third-party extensions.
Looks reasonable offhand.
regards, tom lane
On 10/13/19 12:25 PM, Peter Eisentraut wrote:
diff --git a/contrib/hstore/hstore_compat.c b/contrib/hstore/hstore_compat.c index 1d4e7484e4..d75e9cb23f 100644 --- a/contrib/hstore/hstore_compat.c +++ b/contrib/hstore/hstore_compat.c @@ -299,7 +299,7 @@ hstoreUpgrade(Datum orig)if (valid_new)
{
-#if HSTORE_IS_HSTORE_NEW
+#ifdef HSTORE_IS_HSTORE_NEW
elog(WARNING, "ambiguous hstore value resolved as hstore-new");
Checking the current sources, git history, and various older commits, I
did not find where HSTORE_IS_HSTORE_NEW was ever defined. I expect it
was defined at some point, but I checked back as far as 9.0 (where the
current contrib/hstore was originally committed) and did not see it.
Where did you find this, and can we add a code comment? This one #ifdef
is the only line in the entire repository where this label is used,
making it hard to check if changing from #if was the right decision.
The check on HSTORE_IS_HSTORE_NEW goes back at least as far as 2006,
suggesting it was needed for migrating from some version pre-9.0, making
me wonder if anybody would need this in the field. Should we drop
support for this? I don't have a strong reason to advocate dropping
support other than that this #define appears to be undocumented.
mark
"Mark" == Mark Dilger <hornschnorter@gmail.com> writes:
+#ifdef HSTORE_IS_HSTORE_NEW
Mark> Checking the current sources, git history, and various older
Mark> commits, I did not find where HSTORE_IS_HSTORE_NEW was ever
Mark> defined.
In contrib/hstore, it never was.
The current version of contrib/hstore had a brief life as a separate
extension module called hstore-new, which existed to backport its
functionality into 8.4. The data format for hstore-new was almost
identical to the new contrib/hstore one (and thus different from the old
contrib/hstore), and changed at one point before its final release, so
there were four possible upgrade paths as explained in the comments.
The block comment with the most pertinent explanation seems to have
been a victim of pgindent, but the relevant part is this:
* [...] So the upshot of all this
* is that we can treat all the edge cases as "new" if we're being built
* as hstore-new, and "old" if we're being built as contrib/hstore.
So, HSTORE_IS_HSTORE_NEW was defined if you were building a pgxs module
called "hstore-new" (which was distributed separately on pgfoundry but
the C code was the same), and not if you're building "hstore" (whether
an in or out of tree build).
Mark> The check on HSTORE_IS_HSTORE_NEW goes back at least as far as
Mark> 2006, suggesting it was needed for migrating from some version
Mark> pre-9.0, making me wonder if anybody would need this in the
Mark> field. Should we drop support for this? I don't have a strong
Mark> reason to advocate dropping support other than that this #define
Mark> appears to be undocumented.
The only reason not to remove most of hstore_compat.c is that there is
no way to know what data survives in the wild in each of the three
possible hstore formats (8.4 contrib, pre-final hstore-new, current). I
think it's most unlikely that any of the pre-final hstore-new data still
exists, but how would anyone know?
(The fact that there have been exactly zero field reports of either of
the WARNING messages unfortunately doesn't prove much. Almost all
possible non-current hstore values are unambiguously in one or other of
the possible formats, the ambiguity is only possible because the old
code didn't always set the varlena length to the correct size, but left
unused space at the end.)
--
Andrew (irc:RhodiumToad)
On 2019-10-13 21:25, Peter Eisentraut wrote:
During the cleanup of the _MSC_VER versions (commit
38d8dce61fff09daae0edb6bcdd42b0c7f10ebcd), I found it useful to use
-Wundef, but that resulted in a bunch of gratuitous warnings. Here is a
patch to fix those. Most of these are just stylistic cleanups, but the
change in pg_bswap.h is potentially useful to avoid misuse by
third-party extensions.
committed
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-10-14 17:12, Mark Dilger wrote:
The check on HSTORE_IS_HSTORE_NEW goes back at least as far as 2006,
suggesting it was needed for migrating from some version pre-9.0, making
me wonder if anybody would need this in the field. Should we drop
support for this? I don't have a strong reason to advocate dropping
support other than that this #define appears to be undocumented.
Per subsequent messages in this thread, this issue is outside the scope
of my patch, so I proceeded with my patch as I had proposed it.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 10/15/19 5:23 AM, Andrew Gierth wrote:
"Mark" == Mark Dilger <hornschnorter@gmail.com> writes:
+#ifdef HSTORE_IS_HSTORE_NEW
Mark> Checking the current sources, git history, and various older
Mark> commits, I did not find where HSTORE_IS_HSTORE_NEW was ever
Mark> defined.In contrib/hstore, it never was.
The current version of contrib/hstore had a brief life as a separate
extension module called hstore-new, which existed to backport its
functionality into 8.4. The data format for hstore-new was almost
identical to the new contrib/hstore one (and thus different from the old
contrib/hstore), and changed at one point before its final release, so
there were four possible upgrade paths as explained in the comments.The block comment with the most pertinent explanation seems to have
been a victim of pgindent, but the relevant part is this:* [...] So the upshot of all this
* is that we can treat all the edge cases as "new" if we're being built
* as hstore-new, and "old" if we're being built as contrib/hstore.So, HSTORE_IS_HSTORE_NEW was defined if you were building a pgxs module
called "hstore-new" (which was distributed separately on pgfoundry but
the C code was the same), and not if you're building "hstore" (whether
an in or out of tree build).
I don't really dispute your claim, but it doesn't unambiguously follow
from the wording of the comment. The part that tripped me up while
reviewing Peter's patch is that he changed the preprocessor logic to use
#ifdef rather than #if, implying that he believes HSTORE_IS_HSTORE_NEW
will only be defined when true, and undefined when false, rather than
something like:
#if OLD_STUFF
#define HSTORE_IS_HSTORE_NEW 0
#else
#define HSTORE_IS_HSTORE_NEW 1
#endif
which is admittedly a less common coding pattern than only defining it
when true, but the choice of #if rather than #ifdef in the original
sources might have been intentional.
I tried briefly to download this project from pgfoundry without success.
Do you have a copy of the relevant code where you can see how this
gets defined, and can you include it in a reply?
Thanks,
mark
"Mark" == Mark Dilger <hornschnorter@gmail.com> writes:
Mark> I tried briefly to download this project from pgfoundry without
Mark> success. Do you have a copy of the relevant code where you can
Mark> see how this gets defined, and can you include it in a reply?
I have a backup of the CVS from the pgfoundry version, but the thing is
so obsolete that I had never bothered converting it to git; it hasn't
been touched in 10 years.
The Makefile had this:
PG_CPPFLAGS = -DHSTORE_IS_HSTORE_NEW
The only possible use for this code is if someone were to discover an
old 8.4 install with an old hstore-new module in use. I think the
chances of this are small enough not to be of much concern.
I have put up a CVS->Git conversion for the benefit of software
archaeologists only at: https://github.com/RhodiumToad/hstore-ancient
--
Andrew (irc:RhodiumToad)