Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to xnumber()

[Rasmus Villemoes <linux@rasmusvillemoes.dk>]

On Mon, Dec 28 2015, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>> On Mon, Dec 28 2015, Joe Perches <joe@perches.com> wrote:
>>
>>> On Mon, 2015-12-28 at 20:18 +0200, Andy Shevchenko wrote:
>>>> xnumber() is a special helper to print a fixed size type in a hex format with
>>>> '0x' prefix with padding and reduced size. In the module we have already
>>>> several copies of such code. Consolidate them under xnumber() helper.
>>>>
>>>> There are couple of differences though.
>>>>
>>>> It seems nobody cared about the output in case of CONFIG_KALLSYMS=n when
>>>> printing symbol address because the asked width is not enough to care either
>>>> prefix or last byte. Fixed here.
>>
>> ok, though I'm curious what 'last byte' refers to here?
>
> The last byte ('78') as it appears in the string carrying the number
> '0x12345678'. Yeah, might be confusing, I'm open for suggestion how to
> phrase it.

Maybe just don't mention "last byte" (I thought it was referring to the
final '\0' terminator, and the "78" is actually the first byte on
little-endian, so there's lots of ways to interpret this wrongly...),
and say that the width doesn't take the prefix into account (which is
obviously what has been forgotten).

BTW, thinking a bit more about this, using the field width+ZEROPAD is
arguably wrong. It would be better to set the precision to
2*sizeof(type), since for numeric conversions the precision precisely
means "the minimum number of digits to appear". That also avoids the
annoying interactions with a user-supplied field width, and actually
allows the user to do

%-20pNF

to get "0x00abcdef" padded with 10 spaces on the right (provided we
do pass through the original spec). So I now think xnumber should do

spec.base = 16;
spec.flags |= SMALL | SPECIAL;
spec.precision = 2*size;

Since gcc complains about the 0 flag passed to %p, that will never be
set, so any field width padding either left or right will be by spaces.

But I agree that it should be explicitly documented which %p extensions
accept and honour a field width and which that don't (some out of
necessity, since it's overloaded to pass e.g. a bitmap size or buffer
size).

>>> xnumber isn't a great name.
>>
>> Maybe 'hexnumber'.
>
> We already have similar for %*ph. And as you noticed below…
>
>> That's a bit further away from 'number', and 'x'
>> might stand for something other than hex.
>
> …isn't only about hex. I don't know how to play on words the all three
> flags including 16 base.

It's a helper local to that file, so I'm not too worried about whatever
name is chosen. full_width_lower_case_hexnumber is obviously way too
verbose. I suppose most people instinctively expect hex numbers to be in
lower case, but full_width_hexnumber is still too much. (also, I think
you misinterpreted me: I wasn't complaining about 'x' not saying
everything, I was complaining about 'x' not saying anything at all. IOW,
what I meant was that, taken out of context, a function called 'xnumber'
doesn't immediately tell me that it has anything to do with printing a
number in hexadecimal, so it would be better to spend two more
characters on its name to at least carry one aspect of what it does.).

Rasmus