On Fri, May 19, 2023 at 05:05:20PM +0200, Hanna Czenczek wrote:
> On 12.05.23 04:10, Eric Blake wrote:
> > It's hard to tweak code for consistency if I can't prove what will or
> > won't break from those tweaks. Time to add unit tests for
> > qemu_strtod() and qemu_strtod_finite().
> >
> > Among other things, I wrote a check whether we have C99 semantics for
> > strtod("0x1") (which MUST parse hex numbers) rather than C89 (which
> > must stop parsing at 'x'). These days, I suspect that is okay; but if
> > it fails CI checks, knowing the difference will help us decide what we
> > want to do about it. Note that C2x, while not final at the time of
> > this patch, has been considering whether to make strtol("0b1") parse
> > as 1 with no slop instead of the C17 parse of 0 with slop "b1"; that
> > decision may also bleed over to strtod(). But for now, I didn't think
> > it worth adding unit tests on that front (to strtol or strtod) as
> > things may still change.
> >
> > Likewise, there are plenty more corner cases of strtod proper that I
> > don't explicitly test here, but there are enough unit tests added here
> > that it covers all the branches reached in our wrappers. In
> > particular, it demonstrates the difference on when *value is left
> > uninitialized, which an upcoming patch will normalize.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> >
> > ---
> >
> > v2: Added g_assert_false(signbit(res)) anywhere I used
> > g_assert_cmpfloat(res,==,0.0); add a test for strtod() hex parsing and
> > handling of junk after ERANGE, which is major enough that I dropped
> > R-b
> > ---
> > tests/unit/test-cutils.c | 510 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 510 insertions(+)
> >
> > diff --git a/tests/unit/test-cutils.c b/tests/unit/test-cutils.c
> > index d3076c3fec1..1763839a157 100644
> > --- a/tests/unit/test-cutils.c
> > +++ b/tests/unit/test-cutils.c
>
> [...]
>
> > +static void test_qemu_strtod_erange_junk(void)
> > +{
> > + const char *str;
> > + const char *endptr;
> > + int err;
> > + double res;
> > +
> > + /* EINVAL has priority over ERANGE */
>
> By being placed here, this comment confused me a bit, because the first case
> does return ERANGE. So I’d prefer it above the second case, where we
> actually expect EINVAL, but understand that’s a personal preference. (Same
> for the _finite_ variant)
The test is what happens when both conditions apply. For
qemu_strtod("1e-999junk", &endptr), only ERANGE applies (because
"junk" is returned in endptr); it is not until
qemu_strtod("1e-999junk", NULL) where EINVAL is also possible
(trailing junk takes precedence over underflow). For qemu_strtosz(),
I made it a bit more obvious by writing a helper function that shows
both errno values in a single line, rather than spreading out the
boilerplate over multiple lines.
Should I do a similar helper function for qemu_strtod[_finite] in v3?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
