[PATCH] D132568: [clang][Sema] check default argument promotions for printf

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/test/FixIt/format.m:40
 
-void test_object_correction (id x) {  
+void test_object_correction (id x) {
   NSLog(@"%d", x); // expected-warning{{format specifies type 'int' but the argument has type 'id'}}
----------------
inclyc wrote:
> aaron.ballman wrote:
> > It looks like some whitespace only changes snuck in.
> > It looks like some whitespace only changes snuck in.
> 
> I'm sorry for this, do I need to discard these whitespace-only changes? Since this patch changes this file, is it my responsibility to keep other parts that don't belong to this patch unchanged, or am I correcting them by the way?
> I'm sorry for this, do I need to discard these whitespace-only changes? Since this patch changes this file, is it my responsibility to keep other parts that don't belong to this patch unchanged, or am I correcting them by the way?

You should discard the whitespace only changes; you are welcome to land an NFC commit that fixes just the whitespace issues though!

We typically ask that changes unrelated to the patch summary should be separated out so that each patch is self-contained for just one thing. Not only is that easier to review, it also makes it less likely that we end up reverting good changes if we need to revert a patch.


================
Comment at: clang/test/Sema/format-strings-scanf.c:289-290
+  // ill-formed floats
+  scanf("%hf", // expected-warning{{length modifier 'h' results in undefined behavior or no effect with 'f' conversion specifier}}
+  &sc); // Is this a clang bug ?
+
----------------
inclyc wrote:
> aaron.ballman wrote:
> > Is what a clang bug?
> > 
> > Also, what is with the "or no effect" in that wording? "This could cause major issues or nothing bad at all might happen" is a very odd way to word a diagnostic. :-D (Maybe something to look into improving in a later patch.)
> > Is what a clang bug?
> 
> Look at `sc` which is a `signed char`, but scanf need a pointer `float *`, I add this comment because I think there should be a warning like 
> ```
> format specifies type 'float *' but the argument has type 'signed short *'
> ```
> 
> See printf tests below
Oh I see what you're saying! There are two issues with the code: one is that the format specifier itself is UB and the other is that the argument passed to this confused format specifier does not agree.

To me, the priority is the invalid format specifier; unless the user corrects that, we're not really certain what they were aiming to scan in. And I think it's *probably* more likely that the user made a typo in the format specifier instead of trying to scan a half float (or whatever they thought they were scanning) into a `signed char`, so it seems defensible to silence the mismatched specifier diagnostic.

WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132568/new/

https://reviews.llvm.org/D132568