Re: [Qemu-devel] [PATCH v2 2/4] tests: add some qemu_strtosz() tests

[Eric Blake]

On 09/18/2015 09:10 AM, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 09/18/2015 04:59 AM, address@hidden wrote:
>>> From: Marc-André Lureau <address@hidden>
>>>
>>> While reading the function I decided to write some tests.
>>>
>>> Signed-off-by: Marc-André Lureau <address@hidden>
>>> Reviewed-by: Eric Blake <address@hidden>
>>> ---
>>>  tests/test-cutils.c | 91
>>>  +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 91 insertions(+)
>>
>> I accepted v1 because it was better than no tests at all, but did make
>> some suggestions for additional tests to perform.  I'm surprised you
>> didn't include any of those suggestions in v2.  For example, it would be
>> nice if the testsuite documents a contract on what happens with a bogus
>> suffix: is "1234x" outright rejected, or does it parse as "1234" leaving
>> the pointer at 'x'?
> 
> I thought you were ok with this patch as is.

If there was no other reason for a respin.  But once you are doing a
respin, you might as well consider it, or at least document after the
--- that you at least thought about it and had a reason for not doing it.

> But I can add some failing tests if you want (although it feels like testing 
> strtod() at this point.

Not quite. strtod() doesn't parse suffixes, but does consume input
anyways; it then requires the user to do post-processing (so by itself
it is not an easy contract).  The whole point of writing our own wrapper
is so that we can have sane semantics without the caller having to do
post-processing.  That should include a sane contract for what we do on
a suffix we don't recognize is useful.  In particular, there could be
arguments for both "parse as much as possible", and for "refuse to parse
anything that has trailing garbage"; and since I don't know which way it
currently is, it means we SHOULD be making it part of the contract and
giving it some testing.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature