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

[Markus Armbruster]

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.

I'll review the actual patch now.