Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>> Let's provide a wrapper for strtod().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  2 ++
>>   util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>
>
>> +
>> +/**
>> + * Convert string @nptr to a finite double.
>> + *
>> + * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
>> + * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.
>
> s/rejcted/rejected/

Also, just overflow.  Floating-point underflow is when a computation's
mathematical result is too close to zero to be represented without
extraordinary rounding error.

Skip this paragraph unless you're ready to nerd out.  IEEE 754 section
7.5 defines underflow to happen

    [...] either

    a) after rounding — when a non-zero result computed as though the
    exponent range were unbounded would lie strictly between ±b^emin,
    or

    b) before rounding — when a non-zero result computed as though both
    the exponent range and the precision were unbounded would lie
    strictly between ±b^emin.

where b^emin is the smallest normal number.  

The "Works like qemu_strtoul()" is a bit lazy.  I guess it works like
qemu_strtoul() in the sense that it adds to strtod() what qemu_strtoul()
adds to strtoul().  I consciously didn't take a similar shortcut in
commit 4295f879bec: I documented both qemu_strtol() and qemu_strtoul()
in longhand, and used "Works like" shorthand only where that's actually
the case: qemu_strtoll() works like qemu_strtol(), and qemu_strtoull()
works like qemu_strtoul().  I'd prefer longhand for qemu_strtod().  It
costs us a few lines, but it results in a clearer contract.

>> + */
>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>> +{
>> +    int ret = qemu_strtod(nptr, endptr, result);
>
> On overflow, result is set to HUGE_VAL (aka "inf") with ret set to
> -ERANGE.  (The C standard uses HUGE_VAL rather than directly requiring
> infinity on overflow, in order to cater to museum platforms where the
> largest representable double is still finite; but no one develops qemu
> on a non-IEEE machine these days so we know that HUGE_VAL == INF).

Aside: museum clauses like this one make the standard much harder to
read than necessary.  I wish they'll purge them from C2X.

>> +
>> +    if (!ret && !isfinite(*result)) {
>> +        return -EINVAL;
>> +    }

qemu_strtol() & friends leave *result alone when they return -EINVAL.
This one doesn't.  Unlikely to hurt anyone, but I'd prefer to keep them
consistent.

> This check means that overflow ("1e9999") fails with -ERANGE, while
> actual infinity ("inf") fails with -EINVAL, letting the user
> distinguish between the two.  Still, I wonder if assigning a
> non-finite value into result on -ERANGE is the wisest course of
> action.  We'll just have to see in the next patches that use this.

I guess it's about as "wise" as qemu_strtol() storing LONG_MAX on
integer overflow.

I'm fine with the semantics David picked, as long as they're spelled out
in the function contract.

> With the typo fix,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>