On Tue, Feb 13, 2024 at 10:26 AM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Mon, Feb 12, 2024 at 11:39 PM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote: > > > > From: Kent Overstreet <kent.overstreet@xxxxxxxxx> > > > > The new flags parameter allows controlling > > - Whether or not the units suffix is separated by a space, for > > compatibility with sort -h > > - Whether or not to append a B suffix - we're not always printing > > bytes. And you effectively missed to _add_ the test cases for the modified code. Formal NAK for this, the rest is discussable, the absence of tests is not. > > Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx> > > Signed-off-by: Suren Baghdasaryan <surenb@xxxxxxxxxx> > > It seems most of my points from the previous review were refused... > > ... > > You can move the below under --- cutter, so it won't pollute the git history. > > > Cc: Andy Shevchenko <andy@xxxxxxxxxx> > > Cc: Michael Ellerman <mpe@xxxxxxxxxxxxxx> > > Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx> > > Cc: Paul Mackerras <paulus@xxxxxxxxx> > > Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx> > > Cc: Jason Wang <jasowang@xxxxxxxxxx> > > Cc: "Noralf Trønnes" <noralf@xxxxxxxxxxx> > > Cc: Jens Axboe <axboe@xxxxxxxxx> > > --- > > ... > > > --- a/include/linux/string_helpers.h > > +++ b/include/linux/string_helpers.h > > @@ -17,14 +17,13 @@ static inline bool string_is_terminated(const char *s, int len) > > ... > > > -/* Descriptions of the types of units to > > - * print in */ > > -enum string_size_units { > > - STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > - STRING_UNITS_2, /* use binary powers of 2^10 */ > > +enum string_size_flags { > > + STRING_SIZE_BASE2 = (1 << 0), > > + STRING_SIZE_NOSPACE = (1 << 1), > > + STRING_SIZE_NOBYTES = (1 << 2), > > }; > > Do not kill documentation, I already said that. Or i.o.w. document this. > Also the _SIZE is ambigous (if you don't want UNITS, use SIZE_FORMAT. > > Also why did you kill BASE10 here? (see below as well) > > ... > > > --- a/lib/string_helpers.c > > +++ b/lib/string_helpers.c > > @@ -19,11 +19,17 @@ > > #include <linux/string.h> > > #include <linux/string_helpers.h> > > > > +enum string_size_units { > > + STRING_UNITS_10, /* use powers of 10^3 (standard SI) */ > > + STRING_UNITS_2, /* use binary powers of 2^10 */ > > +}; > > Why do we need this duplication? > > ... > > > + enum string_size_units units = flags & flags & STRING_SIZE_BASE2 > > + ? STRING_UNITS_2 : STRING_UNITS_10; > > Double flags check is redundant. -- With Best Regards, Andy Shevchenko