inclyc added inline comments.
================
Comment at: clang/lib/AST/FormatString.cpp:422
+ case BuiltinType::Bool:
+ // Don't warn printf("%hd", [char])
+ // https://reviews.llvm.org/D66186
----------------
aaron.ballman wrote:
> The comment is talking about passing a `char` (promoted to `int`) then
> printed as a `short` but the check is looking for the type to be printed as
> an `int`; that doesn't seem like a type confusion situation so much as a
> match due to promotion. Should the test below be looking for a short type
> instead of an int type?
> The comment is talking about passing a `char` (promoted to `int`) then
> printed as a `short` but the check is looking for the type to be printed as
> an `int`; that doesn't seem like a type confusion situation so much as a
> match due to promotion. Should the test below be looking for a short type
> instead of an int type?
Sorry for this (sometimes shit happens). There is a lot of redundant logic in
this place, I will upload the corrected code immediately.
================
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 ?
+
----------------
aaron.ballman wrote:
> 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?
```
printf("%hf", // expected-warning{{length modifier 'h' results in undefined
behavior or no effect with 'f' conversion specifier}}
sc); // expected-warning{{format specifies type 'double' but the argument has
type 'signed char'}}
```
Then clang seems to have two different behaviors to `printf` and `scanf`, and
for `printf` it will give the kind of diagnosis I said. I don't think this
problem is particularly serious, because the key point is that using `%hf` UB.
So maybe we can just keep clang as-is? Or we should consider implement the same
warning in `scanf` ?
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
