[cfe-commits] [Patch] Make -Wformat accept printf("%hhx", c); with -funsigned-char (PR12761)

[Hans Wennborg]

On Tue, May 8, 2012 at 6:46 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Yes, in part. I guess there's no "sign agnostic" modifier we should be
> suggesting when the user wrote 'char'?

Hmm, no I don't think so.

> We just have to let them use
> unsigned (o, u, x, X) or signed (d, i) modifiers equally and suggest
> the current correct one should they provide something that's clearly
> wrong (float type, etc).
> But if the user's printing an explicitly signed or unsigned type, is
> it valid for them to use the opposing signedness conversion specifier?

Yes, I believe it's valid, and therefore we don't warn about it. It
might not be what the user intended though, but that's hard to know :/

This is what the '// Check for "compatible types".' part of
ArgTypeResult::matchesType is for.

> (eg: unsigned char a; signed char b; printf("%i %u", a, b); )
> Shouldn't we be correcting those appropriately? It looks like your
> patch doesn't.

No, we won't touch the conversion specifier in that case. The idea is
that we want to apply the minimum amount of corrections to make the
format good enough that we wouldn't warn about it.

I think the main reason is to preserve the user's choice. If the user
wants to print a signed int with "%u", we should let her do it. Or
even more importantly, with "%x". It also seems weird to
"over-correct" the code, i.e. if we're fixing the length modifier it
would be annoying to "fix" the conversion specifier even if it would
have been accepted without warning.

> Even if we should be accepting them, I wouldn't mind seeing some test
> cases for these variations & comments in the test and/or code
> justifying why they're the right thing to do.

There are some tests for this in the file I touched (see e.g. the
check_char function), and some more in
test/Sema/format-strings-fixit.c, but there is certainly room for more
tests in this area.

Thanks,
Hans