On Tue, Nov 26, 2019 at 11:04:41AM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabk...@redhat.com> writes: > > > On Mon, Nov 25, 2019 at 08:20:23AM +0100, Markus Armbruster wrote: > >> Tao Xu <tao3...@intel.com> writes: > >> > >> > Add do_strtomul() to convert string according to different suffixes. > >> > > >> > Reviewed-by: Eduardo Habkost <ehabk...@redhat.com> > >> > Signed-off-by: Tao Xu <tao3...@intel.com> > >> > >> 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
