Subject: Re: bug#48960: stat v8.30 - device number in decimal shown as 16bit number instead of to converted 8bit
From: Pádraig Brady <[email-masked]>
To: Paul Eggert <[email-masked]>, [email-masked], [email-masked]
Date: Mon, 21 Jun 2021 12:07:01 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 20/06/2021 22:38, Paul Eggert wrote:
> Thanks for writing that patch. One minor note:
> 
> On 6/20/21 7:21 AM, Pádraig Brady wrote:
> 
>> +                                    (to_uchar (mod_char) << CHAR_BIT)
>> +                                     + to_uchar (fmt_char),
> 
> Neither mod_char nor fmt_char can be negative (this is guaranteed by the
> C standard since all the relevant constants are in the basic character
> set) so the to_uchar calls are unnecessary.
> 
> Also, this code assumes that 2 * CHAR_BIT <= MIN (INT_WIDTH,
> UINT_WIDTH), something that POSIX requires but the C standard does not;
> it'd be a bit safer (if pedantic) to add 'verify (2 * CHAR_BIT <= MIN
> (INT_WIDTH, UINT_WIDTH));'.
> 
> I'd also change print_stat's arg from unsigned int to int; if you did
> that, you could change the above 'MIN (INT_WIDTH, UINT_WIDTH)' to plain
> 'INT_WIDTH'. (These days we're negative on unsigned types anyway, for
> all the usual reasons....)
> 
> Better yet, pass two char args to print_stat instead of a single int
> portmanteau.

Yes two separate char arguments is cleaner.
I thought there was some reason for the existing unsigned int usage,
but looking I see it was added for commit db42ae78 (to support :X formats),
but then became unneeded with commit c7375c23 (to instead support %.9X formats).

I've pushed both commits now, so marking this as done,

thanks for the review!
Pádraig