Обсуждение: src/include/utils/float.h comment one link stable

hi.

src/include/utils/float.h

/*
 * Routines to provide reasonably platform-independent handling of
 * infinity and NaN
 *
 * We assume that isinf() and isnan() are available and work per spec.
 * (On some platforms, we have to supply our own; see src/port.)  However,
 * generating an Infinity or NaN in the first place is less well standardized;
 * pre-C99 systems tend not to have C99's INFINITY and NaN macros.  We
 * centralize our workarounds for this here.
 */

/*
 * The funny placements of the two #pragmas is necessary because of a
 * long lived bug in the Microsoft compilers.
 * See http://support.microsoft.com/kb/120968/en-us for details
 */
#ifdef _MSC_VER
#pragma warning(disable:4756)
#endif
static inline float4
get_float4_infinity(void)
{
#ifdef INFINITY
    /* C99 standard way */
    return (float4) INFINITY;
#else
#ifdef _MSC_VER
#pragma warning(default:4756)
#endif
    /*
     * On some platforms, HUGE_VAL is an infinity, elsewhere it's just the
     * largest normal float8.  We assume forcing an overflow will get us a
     * true infinity.
     */
    return (float4) (HUGE_VAL * HUGE_VAL);
#endif
}

this link (http://support.microsoft.com/kb/120968/en-us) is stale.

> On 6 Oct 2025, at 14:51, jian he <jian.universality@gmail.com> wrote:

> this link (http://support.microsoft.com/kb/120968/en-us) is stale.

Judging by the cec8394b5ccd3 this was added for MSVC 2013 support, and commit
8fd9bb1d9654c59d bumped the minimum requirement to MSVC 2019, so I wonder if
this at all relevant anymore?

The link does indeed not work anymore, and archive.org hasn't indexed it, so I
guess a better comment at this point would be something like the below to
retain the information for anyone with archeological ambitions.

- * long lived bug in the Microsoft compilers.
- * See http://support.microsoft.com/kb/120968/en-us for details
+ * long lived bug in the Microsoft compilers (KB120968).

--
Daniel Gustafsson

On Tue, 7 Oct 2025 at 02:11, Daniel Gustafsson <daniel@yesql.se> wrote:
> Judging by the cec8394b5ccd3 this was added for MSVC 2013 support, and commit
> 8fd9bb1d9654c59d bumped the minimum requirement to MSVC 2019, so I wonder if
> this at all relevant anymore?

Looks like these were added in cec8394b5 for VS2013 support.

Going by [1] it's talking about a bug relating to having to use the
pragma outside of the function body to disable warnings >= 4700

The warning that's being disabled is talked about in [2] and relates
to floating point overflow. I do get a warning in VS2022 if I try to
compile the fragment in [2] with the flags mentioned:

>cl /W2 /Od test.c
Microsoft (R) C/C++ Optimizing Compiler Version 19.44.35215 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

test.c
test.c(5): warning C4056: overflow in floating-point constant arithmetic

However, I don't seem to be able to get rid of that warning no matter
where I put the #pragma to try to disable it.

As for float.h, I would have assumed it was the "return (float4)
(HUGE_VAL * HUGE_VAL);" that would cause the warning, but the #pragma
to put the warning back to default is before that line, so I'm not
sure what's going on there.

Going by the draft C11 standard in [3], on page 230 it looks like
INFINITY is always defined now, so maybe we can get rid of the other
code and just always "return (float4) INFINITY;"? I did try that with
VS2022 just to make sure and I don't get a warning.

Given the INFINITE stuff seems standard now, tried the attached patch,
and it compiles for me. The comment about the definitions for isnan
and isinf in src/port seem wrong. I didn't find anything in there, so
removed that part.

Here's what I tried. Compiles without warnings in VS2022.

David

[1] https://www.betaarchive.com/wiki/index.php?title=Microsoft_KB_Archive/120968
[2]
https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-2-c4756?view=msvc-170
[3] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf

David Rowley <dgrowleyml@gmail.com> writes:
> Going by the draft C11 standard in [3], on page 230 it looks like
> INFINITY is always defined now, so maybe we can get rid of the other
> code and just always "return (float4) INFINITY;"? I did try that with
> VS2022 just to make sure and I don't get a warning.

+1, let's give that a try.  However, INFINITY has been required since
C99, so please don't change that comment.

I wonder if we couldn't also remove the NAN hacks in the same file.
NAN's been required since C99 as well, and it's equally hard to
believe that we still need to support compilers that don't conform.

(Strictly speaking, NAN is required only if the underlying float
implementation has it.  But we desupported non-IEEE float arithmetic
years ago.)

            regards, tom lane

On Tue, 7 Oct 2025 at 04:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> David Rowley <dgrowleyml@gmail.com> writes:
> > Going by the draft C11 standard in [3], on page 230 it looks like
> > INFINITY is always defined now, so maybe we can get rid of the other
> > code and just always "return (float4) INFINITY;"? I did try that with
> > VS2022 just to make sure and I don't get a warning.
>
> +1, let's give that a try.  However, INFINITY has been required since
> C99, so please don't change that comment.

I got a clean bill of health from CI [1] for this patch, so went ahead
and pushed it after putting the C99 comments back.

The VS2019 CI instance seems happy and free from any new warnings.

> I wonder if we couldn't also remove the NAN hacks in the same file.
> NAN's been required since C99 as well, and it's equally hard to
> believe that we still need to support compilers that don't conform.
>
> (Strictly speaking, NAN is required only if the underlying float
> implementation has it.  But we desupported non-IEEE float arithmetic
> years ago.)

I saw the "The macro NAN is defined if and only if the implementation
supports quiet NaNs for the float type." in the standard and that
causes me to stop at INFINITY. I'd be happy for someone else to
continue. I'm a bit hesitant due to the "#if defined(NAN) &&
!(defined(__NetBSD__) && defined(__mips__))", which are systems I have
little knowledge of.

David

[1] https://cirrus-ci.com/build/5341920676806656

David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, 7 Oct 2025 at 04:24, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I wonder if we couldn't also remove the NAN hacks in the same file.
>> NAN's been required since C99 as well, and it's equally hard to
>> believe that we still need to support compilers that don't conform.
>> 
>> (Strictly speaking, NAN is required only if the underlying float
>> implementation has it.  But we desupported non-IEEE float arithmetic
>> years ago.)

> I saw the "The macro NAN is defined if and only if the implementation
> supports quiet NaNs for the float type." in the standard and that
> causes me to stop at INFINITY.

Well, we know that all supported platforms have quiet NaNs, because
otherwise they'd not be passing our regression tests.  The problem
was just old non-C99-compliant C compilers.  It's *really* hard to
believe that anything supporting C11-or-later doesn't have the NAN
macro.

Given the wording of the spec, I'm slightly tempted to put in

#ifndef NAN
#error "Postgres requires support for IEEE quiet NaNs"
#endif

but it's probably just a waste of effort.

> I'd be happy for someone else to
> continue. I'm a bit hesitant due to the "#if defined(NAN) &&
> !(defined(__NetBSD__) && defined(__mips__))", which are systems I have
> little knowledge of.

Fair enough, I'll take responsibility for that bit.  Might as well
wait to see how the BF likes the INFINITY change, though.

            regards, tom lane

I wrote:
> David Rowley <dgrowleyml@gmail.com> writes:
>> I'd be happy for someone else to
>> continue. I'm a bit hesitant due to the "#if defined(NAN) &&
>> !(defined(__NetBSD__) && defined(__mips__))", which are systems I have
>> little knowledge of.

> Fair enough, I'll take responsibility for that bit.  Might as well
> wait to see how the BF likes the INFINITY change, though.

The three active Windows animals all seem happy with INFINITY,
ditto Solaris and derivatives.  So I'll proceed with changing
the NAN support similarly.

As a side note: I realized while researching the commit history
that there are copies of get_float8_infinity and get_float8_nan
in src/interfaces/ecpg/ecpglib/data.c.  Will take care of those
while at it, and maybe put in a cross-reference to help the
next hacker.

            regards, tom lane