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