On Mon, Dec 28 2015, Andy Shevchenko <[email protected]> wrote:
> On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes > <[email protected]> wrote: >> On Mon, Dec 28 2015, Joe Perches <[email protected]> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
