Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to string_get_size()

[Andy Shevchenko <andy@kernel.org>]

On Tue, Oct 24, 2023 at 03:46:53PM -0400, Kent Overstreet wrote:
> On Tue, Oct 24, 2023 at 05:26:18PM +0300, Andy Shevchenko wrote:
> > On Tue, Oct 24, 2023 at 06:45:58AM -0700, Suren Baghdasaryan wrote:

...

> > >  	string_get_size(nblocks, queue_logical_block_size(q),
> > > -			STRING_UNITS_10, cap_str_10, sizeof(cap_str_10));
> > > +			0, cap_str_10, sizeof(cap_str_10));
> > 
> > This doesn't seem right (even if it works). We shouldn't rely on the
> > implementation details.
> 
> It's now a flags parameter: passing an empty set of flags is not
> "relying on an implementation detail".

0 is the "default" flag which is definitely relies on the "implementation
detail". And I think that it's better that caller will explicitly tell what
they want.

...

> > > -/* 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 {
> > 
> > So, please add UNITS_10 as it is now. It will help if anybody in the future
> > wants to add, e.g., 8-base numbers.
> 
> Octal human readable numbers? No, no one's wanted that so far and I
> very much doubt anyone will want that in the future.

I also in doubt, but still, the explicit is better than implicit in this case
in my opinion.

> > > +	STRING_SIZE_BASE2	= (1 << 0),
> > > +	STRING_SIZE_NOSPACE	= (1 << 1),
> > > +	STRING_SIZE_NOBYTES	= (1 << 2),
> > >  };

...

> > > +enum string_size_units {
> > > +	STRING_UNITS_10,	/* use powers of 10^3 (standard SI) */
> > > +	STRING_UNITS_2,		/* use binary powers of 2^10 */
> > > +};
> > 
> > And what a point now in having these?
> 
> Minimizing the size of the diff and making it more reviewable. It's fine
> as an internal implementation thing.

It's not an issue to rename these all over the places as you already did that
for most of the users. And minimizing diff could be done better with
--histogram algorithm or so. Otherwise it is not an objective here, right?

...

> > I assume you need to split this to a few patches:
> > 
> > 1) rename parameter to be a flags without renaming the definitions (this will
> >    touch only string_helpers part);
> > 2) do the end job by renaming it all over the drivers;
> > 3) add the other flags one-by-one (each in a separate change);
> > 4) use new flags where it's needed;
> 
> No, those would not be atomic changes. In particular changing the
> parameter to a flags without changing the callers - that's not how we do
> things.

> We're currently working towards _better_ type safety for enums, fyi.
> 
> The new flags _could_ be a separate patch, but since it would be
> touching much the same code as the previous patch I don't see the point
> in splitting it.

Individual flags can be discussed, objected or approved and won't affect the
rest of the changes. That's why I highly recommend to reconsider splitting of
this change.

It would be possible to squash back if maintainer wants this, but from review
perspective you are adding more burden to the reviewer's shoulders is not good.

...

> > >  	static const char *const units_10[] = {
> > > -		"B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"
> > > +		"", "k", "M", "G", "T", "P", "E", "Z", "Y"
> > >  	};
> > >  	static const char *const units_2[] = {
> > > -		"B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB", "ZiB", "YiB"
> > > +		"", "Ki", "Mi", "Gi", "Ti", "Pi", "Ei", "Zi", "Yi"
> > >  	};
> > 
> > Ouch, instead of leaving this and actually "cutting the letter" with NO* flags,
> > you did something different.
> 
> Not sure I understand your complaint? Were you attached to the redundant
> Bs?

Flag means "cutting" while in the code you "adding" (doing the opposite). Why
not do exactly "cutting" without touching there. Or since you mentioned changes
across the all callers, make them explicitly tell that they want Bytes suffix.

-- 
With Best Regards,
Andy Shevchenko