On 09/18/2015 09:10 AM, Marc-André Lureau wrote: > Hi > > ----- Original Message ----- >> On 09/18/2015 04:59 AM, marcandre.lur...@redhat.com wrote: >>> From: Marc-André Lureau <marcandre.lur...@redhat.com> >>> >>> While reading the function I decided to write some tests. >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >>> Reviewed-by: Eric Blake <ebl...@redhat.com> >>> --- >>> 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
