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. -Peff
