Re: [PATCH] ls-files: use correct format string
- Date: Thu, 11 Apr 2019 19:49:20 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: Re: [PATCH] ls-files: use correct format string
On Thu, Apr 11, 2019 at 10:28:30PM +0100, Thomas Gummerer wrote:
> > 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.
> 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.