Re: [Qemu-devel] [PATCH 4/7] qapi: String visitor, use %f represenation for floats

[Andreas Färber]

Am 11.05.2012 19:32, schrieb Michael Roth:
> On Fri, May 11, 2012 at 01:34:01PM -0300, Luiz Capitulino wrote:
>> On Fri, 27 Apr 2012 15:21:20 -0500
>> Michael Roth <address@hidden> wrote:
>>
>>> Currently string-output-visitor formats floats as %g, which is nice in
>>> that trailing 0's are automatically truncated, but otherwise this causes
>>> some issues:
>>>
>>>  - it 6 uses significant figures instead of 6 decimal places, which
>>>    means something like 155777.5 (which even has an exact floating point
>>>    representation) will be rounded to 155778 when converted to a string.
>>>
>>>  - output will be presented in scientific notation when the normalized
>>>    form requires a 10^x multiplier. Not a huge deal, but arguably less
>>>    readable for command-line arguments.
>>>
>>>  - due to using sig figs instead of hard-defined decimal places, it
>>>    fails a lot of the test-visitor-serialization unit tests for floats.
>>>
>>> Instead, let's just use %f, which is what the QJSON and the QMP visitors
>>> use.
>>>
>>> Signed-off-by: Michael Roth <address@hidden>
>>> ---
>>>  qapi/string-output-visitor.c       |    2 +-
>>>  tests/test-string-output-visitor.c |    2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
>>> index 92b0305..34e525e 100644
>>> --- a/qapi/string-output-visitor.c
>>> +++ b/qapi/string-output-visitor.c
>>> @@ -52,7 +52,7 @@ static void print_type_number(Visitor *v, double *obj, 
>>> const char *name,
>>>                                Error **errp)
>>>  {
>>>      StringOutputVisitor *sov = DO_UPCAST(StringOutputVisitor, visitor, v);
>>> -    string_output_set(sov, g_strdup_printf("%g", *obj));
>>> +    string_output_set(sov, g_strdup_printf("%f", *obj));
>>
>> Doesn't look like a bug fix worth it for 1.1, am I wrong?
> 
> Well, object_property_print() is the only string-output-visitor user,
> and it's not currently used. I don't see this changing for 1.1, so this
> can probably wait.

Actually it might be in 1.1: there's a pending patch by Paolo to use
that for info qtree, where some properties were missing. My review
comment has been resolved, so I will queue that patch for 1.1 and next.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg