Re: stat signed/unsigned

[Pádraig Brady]

Jim Meyering wrote:
> Pádraig Brady <address@hidden> wrote:
>> Jim Meyering wrote:
>>> Michael Meskes <address@hidden> wrote:
>>>> On Wed, Dec 31, 2008 at 03:18:44PM +0100, Jim Meyering wrote:
>>>>> Thanks, but "man statfs" on linux-based systems shows it can be signed:
>>>>> ...
>>>> Sorry for the noise, I just used grep -r to find stuff like
>>>>
>>>> /usr/include/asm-generic/statfs.h:      __u32 f_files;
>>>> /usr/include/asm-generic/statfs.h:      __u64 f_files;
>>>> /usr/include/asm-generic/statfs.h:      __u64 f_files;
>>> Have you seen ever stat print a negative number
>>> corresponding to that field?  Actually, I'll bet that
>>> *has* happened... and considering the semantics
>>> of that variable, I see no reason to print a signed value.
>> On a related note, I was looking at removing -Wsign-compare warnings
>> and noticed a lot are due to dealing with signed st_size, st_blocks etc.
>> I searched and found no references to where these could ever be negative,
>> and therefore I'm assuming that the type is only really to define the
>> width and should always be interpreted as unsigned.
>>
>> So, I'll add an ST_SIZE() macro to system.h like:
>> #define ST_SIZE(statbuf) ((uintmax_t) (statbuf).st_size)
>> and also add the equivalent cast to ST_NBLOCKS etc.
>>
>> Then any code using them could deal just with unsigned variables,
>> thus avoiding any -Wsign-compare warnings.
> 
> Hi Pádraig,
> 
> That sounds like it could be rather invasive...
>>From an aesthetics/readability point of view, I'm not sure
> I like the idea of using ST_SIZE (st) in place of "st.st_size".

Well it would be more consistent as we already use ST_BLKSIZE etc.
There aren't many references to .st_size really.

> More importantly, there are places in the code that compare stat.st_size
> against negative numbers (at least remove.c).

Yuk, so that code assumes that st_size will be always be signed,
and writes negative numbers in to flag various things.
remove.c is already -Wsign-compare clean so I can just not
use ST_SIZE() there.

> I've resisted making coreutils "-Wsign-compare"-warning free for years,
> because the cost of the required changes seems too high.
> This might be one of those cases where it's better to mark
> known-false-positive warnings and automatically filter them out
> of a separate compilation with just -Wsign-compare.

That's a good suggestion but nearly as much work as fixing up the issues.

> I've started down this clean-up-Wsign-compare-warnings road
> a few times, and inevitably end up concluding it's not worthwhile.

The current patch I have is 50 insertions(+), 37 deletions(-)
So for 13 new lines of code with no new casts I think it
probably is worth addressing.

cheers,
Pádraig.