[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strto
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite() |
Date: |
Tue, 20 Nov 2018 21:07:40 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> Let's provide a wrapper for strtod().
>>
>> Reviewed-by: Eric Blake <address@hidden>
>
> This changed enough from v1 that I would have dropped R-b to ensure
> that reviewers notice the differences.
>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>> include/qemu/cutils.h | 2 ++
>> util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 67 insertions(+)
>>
>
>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>> + * on the sign, and return -ERANGE.
>> + *
>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>> + * sign, and return -ERANGE.
>
> The use of UTF-8 ± in one place but not both is odd. I think we're at
> the point where UTF-8 comments are acceptable these days, rather than
> trying to keep our codebase ASCII-clean, so I don't care which way you
> resolve the inconsistency.
217 out of 6455 git-controlled files contain non-ASCII characters. 53
of them are binary, and don't count. In most text files, it's for
spelling names of authors properly in comments. Ample precedence for
UTF-8 in comments, I'd say.
That said, I second Eric's call for consistency, with the slightest of
preferrences for plain ASCII.
I spotted UTF-8 in two error messages, which might still be unadvisable:
hw/misc/tmp105.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C
is out of range",
hw/misc/tmp421.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C
is out of range",
>> +/**
>> + * Convert string @nptr to a finite double.
>> + *
>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>> + * with -EINVAL and no conversion is performed.
>> + */
>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double
>> *result)
>> +{
>> + double tmp;
>> + int ret;
>> +
>> + ret = qemu_strtod(nptr, endptr, &tmp);
>> + if (ret) {
>> + return ret;
>
> So, if we overflow, we are returning -ERANGE but with nothing stored
> into *result. This is different from qemu_strtod(), where a return of
> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
> That seems awkward.
Violates the contract's "like qemu_strtod()".
>> + } else if (!isfinite(tmp)) {
>> + if (endptr) {
>> + *endptr = nptr;
>> + }
>> + return -EINVAL;
>
> Rewinding back to the start of "inf" is interesting, but matches your
> documentation.
Yes. I like it.
>> + }
>> +
>> + *result = tmp;
>> + return ret;
>> +}
>> +
>
> I think you still need to fix -ERANGE handling before I can give R-b.
[Qemu-devel] [PATCH v2 3/9] qapi: Fix string-input-visitor to reject NaN and infinities, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 4/9] qapi: Use qemu_strtod_finite() in qobject-input-visitor, David Hildenbrand, 2018/11/20
[Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests, David Hildenbrand, 2018/11/20