Re: [PATCH v2 09/19] test-cutils: Add coverage of qemu_strtod

[Hanna Czenczek <hreitz@redhat.com>]

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 <eblake@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)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

> +    str = "1e-999junk";
> +    endptr = "somewhere";
> +    res = 999;
> +    err = qemu_strtod(str, &endptr, &res);
> +    g_assert_cmpint(err, ==, -ERANGE);
> +    g_assert_cmpfloat(res, <=, DBL_MIN);
> +    g_assert_cmpfloat(res, >=, 0.0);
> +    g_assert_false(signbit(res));
> +    g_assert_true(endptr == str + 6);
> +
> +    endptr = "somewhere";
> +    res = 999;
> +    err = qemu_strtod(str, NULL, &res);
> +    g_assert_cmpint(err, ==, -EINVAL);
> +    g_assert_cmpfloat(res, ==, 0.0);
> +    g_assert_false(signbit(res));
> +}