Thread: 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
Attachment
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)