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

[Eduardo Habkost]

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

I agree the interesting part is 2.  I still consider 1 being
refactoring, as it doesn't change any external behavior.

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

Agreed.

> 
> I'll review the actual patch now.

Thanks!

-- 
Eduardo