From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752834AbbL1XBs (ORCPT ); Mon, 28 Dec 2015 18:01:48 -0500 Received: from mail-wm0-f47.google.com ([74.125.82.47]:34980 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752566AbbL1XBq convert rfc822-to-8bit (ORCPT ); Mon, 28 Dec 2015 18:01:46 -0500 From: Rasmus Villemoes To: Andy Shevchenko Cc: Joe Perches , Andrew Morton , "linux-kernel\@vger.kernel.org" Subject: Re: [PATCH v1 1/1] lib/vsprintf: refactor duplicate code to xnumber() Organization: D03 References: <1451326703-122826-1-git-send-email-andriy.shevchenko@linux.intel.com> <1451327112.3219.14.camel@perches.com> <87h9j245ed.fsf@rasmusvillemoes.dk> X-Hashcash: 1:20:151228:andy.shevchenko@gmail.com::2XPNKbwtwCul4HqY:0000000000000000000000000000000000004Zfv Date: Tue, 29 Dec 2015 00:01:43 +0100 In-Reply-To: (Andy Shevchenko's message of "Tue, 29 Dec 2015 00:20:35 +0200") Message-ID: <8760zi41pk.fsf@rasmusvillemoes.dk> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Dec 28 2015, Andy Shevchenko wrote: > On Mon, Dec 28, 2015 at 11:42 PM, Rasmus Villemoes > wrote: >> On Mon, Dec 28 2015, Joe Perches 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