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

[Kent Overstreet]

On Thu, Oct 26, 2023 at 04:12:52PM +0300, Andy Shevchenko wrote:
> 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.

Andy: to be blunt, you've been pretty hostile and hysterical ("breaking
the kernel!" over debug statements? really?) and the bikeshedding is
getting to be too much - I'm just going to drop this patch from the
series and we'll post process the output as needed.