Re: [PATCH v17 03/14] util/cutils: refactor do_strtosz() to support suffixes list

[Daniel P . Berrangé]

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 :|