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

[Markus Armbruster]

Daniel P. Berrangé <address@hidden> writes:

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

Since this patch only prepares for new features, it doesn't need test
updates.  The next patch adds features, and it does update the tests.