On 12/27/19 4:41 AM, Erich E. Hoover wrote:
On Thu, Dec 26, 2019 at 7:05 AM Piotr Caban piotr.caban@gmail.com wrote:
... I was working on different strtod implementation (ucrtbase (as well as glibc) uses sub 0.5 ulp precision algorithm). It's not ready yet. Also it's not something that can be added during code-freeze.
Yeah, I was struggling to get something that passes this scanf test (without breaking the more numerous strtod tests): === ret = vsscanf_wrapper(tests[i], "1.1e-30", "%lf", &double_res); ok(ret == 1, "sscanf returned %d for flags %#x\n", ret, tests[i]); ok(double_res == 1.1e-30, "got wrong double %.16e for flags %#x\n", double_res, tests[i]); === I finally came up with the attached the other day, which seems to do the trick*. If you think AJ might be okay with it then would only leave webservices/reader.c:str_to_double with "long double" usage, which I now have a patch for implementing with scanf.
The 1.1e-30 tests is an example of a test that stopped working in strtod with your patches. While working on new implementation of strtod I was running Chromium string conversion tests (it's a 100k semi random doubles conversion test). Here are some results: ucrtbase: - Windows: 0 failures - Current 32-bit Wine: 32062 failures, biggest error: 3 ulp - 6e7f357 32-bit Wine: 36 failures, biggest error: 1 ulp msvcrt: - Windows 10: 16 failures
ucrtbase scanf tests: - Windows: 0 failures - Current 32-bit Wine: 36 failures, biggest error: 1 ulp - Current 32-bit Wine with the patch you have attached: 49779 failures, the exponent is sometimes off by 1, e.g. on "1.5475148155234508e-252"
The test that produces the biggest error for strtod function is: 4.0621786324484882e-053
I'm afraid that your patches will possibly cause tons of regressions and I'm not sure yet how to proceed with it during the code-freeze. There's a quite simple algorithm that should have similar results as msvcrt (I will need to implement it in order to know) that can possibly go into Wine during code-freeze. On the other hand it will need to be rewritten after the code-freeze so I'm not a fun of it. The proper algorithm I was working on will definitely exceed 1k lines of code.
I was not working on scanf changes. It definitely makes sense to not duplicate the code.
I've been poking at this a little bit and it looks like scanf supports everything that strtod and wcstod need. There are a couple things that our scanf implementation doesn't support yet (inf/nan, Fortran-style exponent notation, and hex notation for floats), but once those are in place we should be good to go. So, it may make sense to centralize everything in scanf (rather than the other way around) so that there isn't any duplication for wide character support.
I was thinking about having a common helper that will be used both in strtod and scanf. Proper string to double conversion is complicated and I don't think it should be mixed with scanf implementation.
Changing the strtod_helper signature to something like: double strtod_helper(MSVCRT_wchar_t get(void *ctx), void unget(MSVCRT_wchar_t c, void *ctx), void *ctx, MSVCRT__locale_t locale, int *err) should make it usable both for strtod like functions and scanf.
- Also works well enough that the new compare_double should not be
necessary, unless you've come up with some more test cases ;)
It will be still needed in string.c:3074. The test is written in a way that exact comparison will not work on Windows. It would probably also make sense to add most conversion tests to ucrtbase because other versions have bugs that we probably don't want to reproduce.
Thanks, Piotr