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

[Hanna Czenczek <hreitz@redhat.com>]

On 19.05.23 19:52, Eric Blake wrote:
> 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 <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)
> 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).

Yep; it’s just that because the comment is directly above one test case, 
I assumed it applied to just that case, and was looking for the EINVAL 
there.  Only then I realized that EINVAL won’t occur there, and the 
comment instead points out the difference between the two cases there are.

> 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?

I mean, from my perspective, all I can see is that it would make 
reviewing v3 more tedious…