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

[David Blaikie]

On Tue, May 8, 2012 at 10:22 AM, Hans Wennborg <hans at chromium.org> wrote:
> On Tue, May 8, 2012 at 4:26 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> There's a few unrelated whitespace changes here - those are usually
>> avoided (changing only whitespace on lines your patch is touching
>> anyway) or at least committed separately to keep the revision history
>> more clear.
>
> OK, I'll take those out of the patch.
>
>> Also, your test case only tests one case, but the code change modifies
>> at least a couple fo the switch cases - it'd be nice to test all your
>> changes. If possible, please add the test cases to an existing test
>> file (I assume there are existing test cases for these warnings)
>> rather than adding a new one. Though I realize you need to pass extra
>> flags - they may not conflict with the existing test cases (or perhaps
>> they do - I'm not sure)
>
> That's a good idea. Moving it to test/Sema/format-strings.c.
>
>> & one naive question: I assume there's no way to detect that the user
>> wrote the type 'char', not 'unsigned char' or 'signed char'?
>
> Yes, those are different builtin types, if that's what you mean.
> 'signed char' and 'unsigned char' are BuiltinType::SChar and UChar,
> respectively. Just 'char' is BuiltinType::Char_S or Char_U depending
> whether its signed or unsigned. Is that what you mean?

Yes, in part. I guess there's no "sign agnostic" modifier we should be
suggesting when the user wrote 'char'? 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?
(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.

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.

- David