Re: [PATCH] ls-files: use correct format string

From: Jeff King <peff@peff.net>
Date: Apr 11, 2019, 23:49 UTC
Message-ID: <20190411234920.GA27914@sigill.intra.peff.net>
In-Reply-To: <20190411212830.GF32487@hank.intra.tgummerer.com>

On Thu, Apr 11, 2019 at 10:28:30PM +0100, Thomas Gummerer wrote:

Show 8 quoted lines

> > I didn't see any comment on this, but it seems like it must be obviously
> > correct, since as you note we do define those fields as unsigned. I'm
> > really surprised that -Wformat doesn't catch this, though. I wonder why.
> 
> Good point.  A bit of digging led me to -Wformat-signedness, which
> should catch this.  This turns up a lot of errors in our codebase.  I
> didn't go through to see how many of them are actual errors, and how
> many are false-positives though.

Ah, right, I totally forgot that signedness got its own warning class. Thanks for enlightening me.

Show 9 quoted lines

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65446 describes how the
> option can lead to false positives, e.g.
> 
>     printf ("%u\n", unsigned_short);
> 
> might turn up an error.  From a quick test this seems to work
> correctly with gcc 8.2.1 that I have on my machine though, so the
> issue might be fixed in newer gcc version, even though that bug report
> is still marked as new.

Interesting. Looking at that thread, I actually don't think it would be so bad to warn there anyway. It's true that due to integer promotion an unsigned short will work with %u, but I'd be just as happy to switch such a format to "%hu", which is more correct.

> Maybe it's worth going through the warnings at some point to see if it
> would be possible to turn -Wformat-signedness on.

I skimmed over a few of the results. There are definitely some that could produce funny output. There are also many that are harmless (e.g., printing a constant 0 with "%o", which technically should be "0U"). I don't think it's high priority, but if anybody wants to chip away at it, be my guest.

In the meantime, I think your patch here is an obvious improvement.

-Peff