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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
