Обсуждение: draft patch for strtof()
As discussed in the Ryu thread, herewith a draft of a patch to use strtof() for float4 input (rather than using strtod() with its double-rounding issue). An exhaustive search shows that this does not change the resulting bit-pattern for any input string that could have been generated by PG with extra_float_digits=3 set. The risk is that values generated by other software, especially code that uses shortest-exact float output (as a number of languages seem to do, and which PG will do if the Ryu patch goes in) will be incorrectly input; though it appears that only one value (7.038531e-26) is both a possible shortest-exact representation and a rounding error (though a number of other values round incorrectly, they are not shortest representations). This includes a fallback to use strtod() the old way if the platform lacks strtof(). A variant file for the new regression tests is needed for such platforms; I've taken a stab at setting this up for the one platform we know will need it (if there are others, the buildfarm will let us know in due course). -- Andrew (irc:RhodiumToad)
Вложения
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> As discussed in the Ryu thread, herewith a draft of a patch to use
> strtof() for float4 input (rather than using strtod() with its
> double-rounding issue).
The errno handling in strtof seems bizarrely overcomplex; why do
you need the separate caller_errno variable?
> This includes a fallback to use strtod() the old way if the platform
> lacks strtof(). A variant file for the new regression tests is needed
> for such platforms; I've taken a stab at setting this up for the one
> platform we know will need it (if there are others, the buildfarm will
> let us know in due course).
I'm not that much on board with introducing test cases that we know,
beyond question, are going to be portability headaches. What will
we actually gain with this, compared to just not having the test case?
I can't see that it's worth either the buildfarm cycles or the human
maintenance effort just to prove that, yes, some platforms have
portability corner cases. I also don't like the prospect that we
ship releases that will fail basic regression tests on platforms
we haven't tested. Coping with such failures is a large burden
for affected packagers or end users, especially when the only useful
"coping" mechanism is to ignore the regression test failure. Might
as well not have it.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> As discussed in the Ryu thread, herewith a draft of a patch to use >> strtof() for float4 input (rather than using strtod() with its >> double-rounding issue). Tom> The errno handling in strtof seems bizarrely overcomplex; why do Tom> you need the separate caller_errno variable? Eh. I was preserving the conventional behaviour of setting errno only on errors and not resetting it to 0, but I suppose you could argue that that is overkill given that the function is called only in one place that's supposed to have already set errno to 0. (And yes, I missed a couple of files and the windows build breaks, working on those) >> This includes a fallback to use strtod() the old way if the platform >> lacks strtof(). A variant file for the new regression tests is needed >> for such platforms; I've taken a stab at setting this up for the one >> platform we know will need it (if there are others, the buildfarm will >> let us know in due course). Tom> I'm not that much on board with introducing test cases that we Tom> know, beyond question, are going to be portability headaches. What Tom> will we actually gain with this, compared to just not having the Tom> test case? The purpose of the test case is to ensure that we're getting the right values on input. If these values fail on any platform that anyone actually cares about, then I think we need to know about it. -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> The errno handling in strtof seems bizarrely overcomplex; why do
> Tom> you need the separate caller_errno variable?
> Eh. I was preserving the conventional behaviour of setting errno only on
> errors and not resetting it to 0,
Oh, I see. -ENOCAFFEINE.
> but I suppose you could argue that
> that is overkill given that the function is called only in one place
> that's supposed to have already set errno to 0.
Well, we probably oughtta endeavor to maintain compatibility with
the function's standard behavior, because other calls to it are
likely to creep in over time. Objection withdrawn.
> Tom> I'm not that much on board with introducing test cases that we
> Tom> know, beyond question, are going to be portability headaches. What
> Tom> will we actually gain with this, compared to just not having the
> Tom> test case?
> The purpose of the test case is to ensure that we're getting the right
> values on input. If these values fail on any platform that anyone
> actually cares about, then I think we need to know about it.
Meh. I think the actual outcome will be that we define any platform
that gets the wrong answer as one that we don't care about, mainly
because we won't have any practical way to fix it. That being the
situation, trying to maintain a test case seems like pointless
make-work.
(FWIW, I'm running the patch on gaur's host, just to confirm it
does what you expect. Should have an answer in an hour or so ...)
regards, tom lane
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> See if Windows likes this one any better.
Doubtful, because AFAICT it's the same patch.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> See if Windows likes this one any better. Tom> Doubtful, because AFAICT it's the same patch. Sigh. copied wrong file. Let's try that again. -- Andrew (irc:RhodiumToad)
Вложения
I wrote:
> (FWIW, I'm running the patch on gaur's host, just to confirm it
> does what you expect. Should have an answer in an hour or so ...)
It does --- it compiles cleanly, and the float4 output matches
float4-misrounded-input.out.
(This is the v1 patch not v2.)
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> (FWIW, I'm running the patch on gaur's host, just to confirm it >> does what you expect. Should have an answer in an hour or so ...) Tom> It does --- it compiles cleanly, and the float4 output matches Tom> float4-misrounded-input.out. Well I'm glad _something_ works. Because it turns out that Windows (at least the version running on Appveyor) completely fucks this up; strtof() is apparently returning infinity or zero _without setting errno_ for values out of range for float: input of "10e70" returns +inf with no error, input of "10e-70" returns (exactly) 0.0 with no error. *facepalm* Any windows-users have any idea about this? -- Andrew (irc:RhodiumToad)
>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes: Andrew> Because it turns out that Windows (at least the version running Andrew> on Appveyor) completely fucks this up; strtof() is apparently Andrew> returning infinity or zero _without setting errno_ for values Andrew> out of range for float: input of "10e70" returns +inf with no Andrew> error, input of "10e-70" returns (exactly) 0.0 with no error. This bug turns out to be dependent on compiler/SDK versions, not surprisingly. So far I have figured out how to invoke these combinations on appveyor: VS2013 / SDK 7.1 (as per cfbot): fails VS2015 / SDK 8.1: works Trying to figure out how to get other combinations to test. -- Andrew (irc:RhodiumToad)
This one builds ok on appveyor with at least three different VS versions. Though I've not tried the exact combination of commands used by cfbot... yet. -- Andrew (irc:RhodiumToad)
Вложения
Merge back in some code changes made in the Ryu patch that really belong here, in preparation for rebasing Ryu on top of this (since this is really a separate functional change). Posting this mainly to let cfbot take a look at it. -- Andrew (irc:RhodiumToad)