Messages in this thread | | | Date | Tue, 24 Oct 2023 15:46:53 -0400 | From | Kent Overstreet <> | Subject | Re: [PATCH v2 01/39] lib/string_helpers: Add flags param to string_get_size() |
| |
On Tue, Oct 24, 2023 at 05:26:18PM +0300, Andy Shevchenko wrote: > (Minimized the list of people for my review / comments) > > On Tue, Oct 24, 2023 at 06:45:58AM -0700, Suren Baghdasaryan wrote: > > From: Kent Overstreet <kent.overstreet@linux.dev> > > > > 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. > > ... > > > 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".
> > -/* 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.
> > + STRING_SIZE_BASE2 = (1 << 0), > > + STRING_SIZE_NOSPACE = (1 << 1), > > + STRING_SIZE_NOBYTES = (1 << 2), > > }; > > Please, add necessary comments.
That I can do.
> > +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.
> > 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.
> > 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?
> Now the main part. Since in 50+% cases (I briefly estimated, it may be more) > this is used in printf() why not introducing a new pointer extension for that? > > Yes, it may be done separately, but it will look like a double effort to me. > Instead it might give us a possibility to scale w/o touching users each time > we want to do something and at the same time hide this complete API under > printf() implementation.
No, I would not be in favor of another %p extension: in particular, since this takes integer inputs the lack of type safety for %p extensions coupled with C's very relaxed approach to integer type conversion would be a really nasty footgun.
| |