Compiler warnings about zic.c in 2026b
I'm observing a few compiler warnings when building 2026b with MSVC 19.50.35726: ../pgsql/src/timezone/zic.c(353): warning C5287: operands are different enum types '<unnamed-enum-RF_NAME>' and '<unnamed-enum-LF_TARGET>'; use an explicit cast to silence this warning ../pgsql/src/timezone/zic.c(353): warning C5287: operands are different enum types '<unnamed-enum-RF_NAME>' and '<unnamed-enum-LP_YEAR>'; use an explicit cast to silence this warning This is with a somewhat modified file, but what it's pointing at is the same in the released 2026b sources: /* The maximum number of fields on any of the above lines. (The "+"s pacify gcc -Wenum-compare.) */ enum { MAX_FIELDS = max(max(+RULE_FIELDS, +LINK_FIELDS), max(+LEAP_FIELDS, +EXPIRES_FIELDS)) }; Perhaps instead of the "+"s, do explicit casts to int? ../pgsql/src/timezone/zic.c(1654): warning C4146: unary minus operator applied to unsigned type, result still unsigned This one is unhappy about /* The largest uintmax_t that is a multiple of BASE**6. Any random uintmax_t value that is this value or greater, yields a biased remainder when divided by BASE**6. UNFAIR_MIN equals the mathematical value of ((UINTMAX_MAX + 1) - (UINTMAX_MAX + 1) % BASE**6) computed without overflow. */ uint_fast64_t unfair_min = - ((UINTMAX_MAX % base__6 + 1) % base__6); Don't immediately have a suggestion about silencing this one. (This one is showing up on substantially older MSVC versions than the first two, as far back as MSVC 19.29.30159. I don't see these warnings on any non-Microsoft compiler.) regards, tom lane
On 2026-07-03 13:09, Tom Lane via tz wrote:
MSVC 19.50.35726:
../pgsql/src/timezone/zic.c(353): warning C5287: operands are different enum types '<unnamed-enum-RF_NAME>' and '<unnamed-enum-LF_TARGET>'; use an explicit cast to silence this warning ... MAX_FIELDS = max(max(+RULE_FIELDS, +LINK_FIELDS), ... Perhaps instead of the "+"s, do explicit casts to int?
C casts are too powerful and error-prone, so I'd rather avoid that. Suppose you change "+" to "0+", or to "- -". Does that pacify MSVC? If we have to, I suppose we can change it to "1+" and subtract 1 later, but I hope things don't go that far. Also, can you send a bug report to the MSVC developers? That warning is incorrect, as ISO C99 says that +RULE_FIELDS and +LINK_FIELDS are both of type 'int' even though RULE_FIELDS and LINK_FIELDS themselves are of enum types.
../pgsql/src/timezone/zic.c(1654): warning C4146: unary minus operator applied to unsigned type, result still unsigned ... uint_fast64_t unfair_min = - ((UINTMAX_MAX % base__6 + 1) % base__6);
Don't immediately have a suggestion about silencing this one. (This one is showing up on substantially older MSVC versions than the first two, as far back as MSVC 19.29.30159.
If it's just an old compiler I would just ignore the warnings. Although I normally use GCC I don't bother pacifying old GCC versions; life is too short to worry about bogus warnings from buggy old compilers.
On 2026-07-04 01:53, Paul Eggert via tz wrote:
Suppose you change "+" to "0+", or to "- -". Does that pacify MSVC?
I asked Gemini, and it replied that these hacks probably won't work. It also said that the bug with MSVC's enum warnings (C5287) being generated for benign expressions like +X < +Y has been reported to Microsoft. A Google search suggests that a fix is on the way, giving this URL: https://developercommunity.visualstudio.com/t/False-positive-C5287:-operands... ... a URL that I don't have access to read, unfortunately. Gemini also suggested using the MSVC compiler switch /wd5287 to work around the compiler bug in the meantime. Please give that a try, or perhaps the URL's target can suggest other workarounds.
Paul Eggert <eggert@cs.ucla.edu> writes:
I asked Gemini, and it replied that these hacks probably won't work. It also said that the bug with MSVC's enum warnings (C5287) being generated for benign expressions like +X < +Y has been reported to Microsoft. A Google search suggests that a fix is on the way, giving this URL:
https://developercommunity.visualstudio.com/t/False-positive-C5287:-operands...
... a URL that I don't have access to read, unfortunately.
Hmm ... I don't have any special access either, but I was able to read https://developercommunity.visualstudio.com/t/warning-C5287:-operands-are-di... which says specifically that this gave a warning as of MSVC 19.44.34918.1: ee = (int)((int)e1_1 | (int)e2_1) and that Microsoft acknowledged that as a bug and fixed it in some later release (doesn't say which unfortunately). This isn't exactly the situation zic.c presents, but it seems likely that it'd apply. I don't see anything indicating that they made it be silent without a cast; maybe they did but there's no evidence of that here. I'm not really following your aversion to a cast. Surely none of these constants will ever come close to exceeding INT_MAX. And I don't buy that applying "+" is less of a hack. Anyway, on the other point: the unsigned warning isn't only on old MSVC versions. I see it also on the latest that we have in the Postgres buildfarm, 19.50.35726. I only meant to comment that it appears on a wider range of MSVC versions than the inconsistent-enum-types one. regards, tom lane
Tom Lane wrote:
and that Microsoft acknowledged that as a bug and fixed it in some later release (doesn't say which unfortunately).
I’m seeing a tag at the bottom that says “Fixed In: 18.0.0,” along with a “C++” tag, so plus a footnote: “This issue is read only, because it has been in the Closed - Fixed state for over 90 days. It was closed for 235 days.” I assume “18.0.0” is a Visual Studio reference, which would put it at 2025-11-11. -- Doug Ewell, CC, ALB | Lakewood, CO, US | ewellic.org (making a rare appearance on the “C minutiae” side of the tz mailing list)
On 2026-07-04 10:38, Tom Lane wrote:
I'm not really following your aversion to a cast. Surely none of these constants will ever come close to exceeding INT_MAX.
Although a cast would be safe in this case, verifying that it's safe requires too much thinking. I want a knowledgeable reader to be able to look at just that line of code and trivially say "yeah, that's benign" while muttering a few choice words about clueless compilers. How about replacing "+" with "~~"? That is, something like the first attached draft patch. It pacifies gcc -Wenum-compare. Does it also pacify MSVC? If so, I can install it.
the unsigned warning isn't only on old MSVC versions. I see it also on the latest that we have in the Postgres buildfarm, 19.50.35726
Ah, OK, I misunderstood. Does the second attached draft patch pacify MSVC? If so, I can install it too. I took a quick look at the postgres copy of zic.c and have a couple of thoughts. First, most of the changes are indenting or comment reformatting and whatnot, which makes it hard to see what's going on. Do you know offhand whether there are any functional changes that tzcode should buy back? If so, are those changes public domain? Second, I don't envy you the job of merging tzcode's zic.c changes into postgres. If there are no functional changes, how about the idea of postgres instead compiling a file pgzic.c that looks something like this: #include "postgres_fe.h" #include "pg_getopt.h" /* Any other Postgres-specific stuff goes here. */ #include "zic.c" /* A byte-for-byte copy of tzcode zic.c. */ ... and using that for the Postgres zic? I could make some changes to tzcode zic.c to make this happen, much as I already did (and want to continue to do) for FreeBSD.
El El sáb, 4 jul 2026 a las 21:10, Paul Eggert via tz <tz@iana.org> escribió:
On 2026-07-04 10:38, Tom Lane wrote:
I'm not really following your aversion to a cast. Surely none of these constants will ever come close to exceeding INT_MAX.
Although a cast would be safe in this case, verifying that it's safe requires too much thinking. I want a knowledgeable reader to be able to look at just that line of code and trivially say "yeah, that's benign" while muttering a few choice words about clueless compilers.
How about replacing "+" with "~~"? That is, something like the first attached draft patch.
That requires much more thinking than (int) IMHO. Guillermo
Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com> writes:
El El sáb, 4 jul 2026 a las 21:10, Paul Eggert via tz <tz@iana.org> escribió:
Although a cast would be safe in this case, verifying that it's safe requires too much thinking. I want a knowledgeable reader to be able to look at just that line of code and trivially say "yeah, that's benign" while muttering a few choice words about clueless compilers.
How about replacing "+" with "~~"? That is, something like the first attached draft patch.
That requires much more thinking than (int) IMHO.
Yeah, I don't think any of these alternatives --- including the existing "+" --- is really simpler than a cast. Fundamentally, what this statement wants to do is arithmetic on these constants, so how is it not treating them as integers at bottom? I'm not really in a position to test whether MSVC would be happy with this. I don't use Windows, so my only test option would be to commit the change into Postgres and see what our buildfarm does with it, which is churn I'd rather not put into our commit history. regards, tom lane
On 2026-07-04 14:29, Tom Lane wrote:
Fundamentally, what this statement wants to do is arithmetic on these constants, so how is it not treating them as integers at bottom?
Yes, the cast treats the enum (which is an integer) as an int (which is also an integer, though of a different integer type). And if the cast did only that it would be fine. But in C, casts can do more than change the type of a value: they can also change the the value itself (e.g., by yielding the low order bits of a pointer), which is definitely not wanted here. I'd rather not impose an extra burden on readers to verify that the cast is OK because the identifier is not a pointer and is not floating point and has a value in int range. Of course sometimes a cast is needed for other reasons, and then C code should use a cast despite casts' hazards and hassles; but here a cast is not needed.
I'm not really in a position to test whether MSVC would be happy with this.
Perhaps someone else can try the two patches I mentioned earlier. Another possibility is that builders using MSVC could add /wd4146 and /wd5287 to the compiler flags. That should pacify MSVC without needing to change tzcode. In the meantime there's no rush to apply those two patches, as zic's behavior is fine; this is merely about having compilers generate fewer bogus warnings. I do take the point, though, that the motivation for using unary "+" instead of a cast is not immediately obvious to all, so I installed the attached patch which I hope helps make this clearer.
In the same vein of compiler-ish warnings, today we got our first Coverity report including the 2026b tzcode, and it complained about
CID 1650305: Error handling issues (CHECKED_RETURN) Calling "remove(tempname)" without checking return value. This library function may fail and return an error code.
1762 remove(tempname);
in rename_dest() and again in close_file(). Perhaps it would be appropriate to report the original error, and then try the remove() and warn if that fails? It's not hugely important since zic is about to fail in both cases, but it might be a good idea to let the user know we left a junk file behind. regards, tom lane
Paul Eggert <eggert@cs.ucla.edu> writes:
I took a quick look at the postgres copy of zic.c and have a couple of thoughts.
First, most of the changes are indenting or comment reformatting and whatnot, which makes it hard to see what's going on. Do you know offhand whether there are any functional changes that tzcode should buy back?
No; I would have submitted patches to tzcode if I thought so. Most of the delta comes from (1) running the code through our formatter "pgindent" or (2) wanting to rely on our autoconf/meson infrastructure instead of a bunch of #if tests to make configuration choices. I suppose that (1) is not really essential, as we do have the mechanism to exclude certain files from pgindent. But everybody who looks at Postgres code is pretty used to our rules and tends to find stuff formatted otherwise to be hard to read --- beauty is in the eye of the beholder. (2) is a deliberate policy choice to reduce portability headaches.
Second, I don't envy you the job of merging tzcode's zic.c changes into postgres.
It's not fun, but what I typically do is apply pgindent to upstream tzcode and then diff against what we have, so it's not as bad as you probably think. regards, tom lane
participants (4)
-
Doug Ewell -
Guillermo Rodriguez Garcia -
Paul Eggert -
Tom Lane