aaron.ballman 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
----------------
inclyc wrote:
> 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.
No worries!
================
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:
> > 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` ?
> So maybe we can just keep clang as-is?
I think let's keep clang as-is for the moment so we can keep the concerns
separate.
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
