[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v17 03/14] util/cutils: refactor do_strtosz() to support suff
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v17 03/14] util/cutils: refactor do_strtosz() to support suffixes list |
Date: |
Tue, 26 Nov 2019 10:33:15 +0000 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
On Tue, Nov 26, 2019 at 11:04:41AM +0100, Markus Armbruster wrote:
> Eduardo Habkost <address@hidden> writes:
>
> > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote:
> >> Tao Xu <address@hidden> writes:
> >>
> >> > Add do_strtomul() to convert string according to different suffixes.
> >> >
> >> > Reviewed-by: Eduardo Habkost <address@hidden>
> >> > Signed-off-by: Tao Xu <address@hidden>
> >>
> >> What's the actual change here? "Refactor" suggests the interfaces stay
> >> the same, only their implementation changes. "Support suffixes list"
> >> suggests some interface supports something new.
> >
> > 1) Parameters added to suffix_mul() (suffix table);
> > 2) do_strtomul() is being extracted from do_strtosz().
> >
> > do_strtomul() interface and behavior stays the same.
>
> Alright, it's two related changes squashed together (which tends to
> complicate writing good commit messages). 2) is really a refactoring.
> 1) is not: it makes suffix_mul() more flexible. Summarizing 1) and 2)
> as "refactor do_strtosz() to support suffixes list" is confusing,
> because it's about 1), while the interesting part is actually 2).
>
> Moreover, the commit message should state why these two changes are
> useful. It tries, but "to support suffixes list" merely kicks the can
> down the road, because the reader is left to wonder why supporting
> suffix lists is useful. It's actually for use by the next patch. So
> say that.
Test case additions to test-cutils.c would go a long way to illustrating
what is added & that its working correctly.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [PATCH v17 03/14] util/cutils: refactor do_strtosz() to support suffixes list, Markus Armbruster, 2019/11/26
[PATCH v17 01/14] util/cutils: Add Add qemu_strtold and qemu_strtold_finite, Tao Xu, 2019/11/22
Re: [PATCH v17 01/14] util/cutils: Add Add qemu_strtold and qemu_strtold_finite, Markus Armbruster, 2019/11/25