nickdesaulniers added a comment.
In D132568#3747971 <https://reviews.llvm.org/D132568#3747971>, @inclyc wrote:
>> Do we want to encode that in `test_promotion` in
>> `clang/test/Sema/format-strings.c`? Seems like tests on shorts are missing.
>
> Tests for short and char "incompatibility" could be found elsewhere in this
> file.
>
> format-strings.c
>
> void should_understand_small_integers(void) {
> printf("%hhu", (short) 10); // expected-warning{{format specifies type
> 'unsigned char' but the argument has type 'short'}}
> printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
> printf("%hu\n", (uint8_t)1); // warning with -Wformat-pedantic only
> }
> /* ... */
> void test13(short x) {
> char bel = 007;
> printf("bel: '0%hhd'\n", bel); // no-warning
> printf("x: '0%hhd'\n", x); // expected-warning {{format specifies type
> 'char' but the argument has type 'short'}}
> }
>
> Do I need to explicitly test again in the `test_promotion`?
Feel free to delete or move existing test cases if they make more sense to
remain together in your added block, rather than more isolated in other test
functions.
================
Comment at: clang/lib/AST/FormatString.cpp:401
+ if (const auto *BT = argTy->getAs<BuiltinType>()) {
+ if (!Ptr) {
+ switch (BT->getKind()) {
----------------
aaron.ballman wrote:
> It's a bit strange that we have two switches over the same `BT->getKind()`
> and the only difference is `!Ptr`; would it be easier to read if we combined
> the two switches into one and had logic in the individual cases for `Ptr` vs
> not `Ptr`?
I almost made the same recommendation myself. For the below switch pair, and
the pair above.
================
Comment at: clang/lib/AST/FormatString.cpp:408-410
+ T == C.ShortTy || T == C.UnsignedShortTy) {
+ return MatchPromotion;
+ }
----------------
nickdesaulniers wrote:
> `{}` aren't necessary
make sure to mark these done in phabricator UI
================
Comment at: clang/test/Sema/format-strings-scanf.c:271-272
+ scanf("%llx", &i); // expected-warning{{format specifies type 'unsigned long
long *' but the argument has type 'int *'}}
+ scanf("%hf", // expected-warning{{length modifier 'h' results in undefined
behavior or no effect with 'f' conversion specifier}}
+ &sc); // Is this a clang bug ?
+ scanf("%s", i); // expected-warning{{format specifies type 'char *' but the
argument has type 'int'}}
----------------
aaron.ballman wrote:
> nickdesaulniers wrote:
> > Hmm...
> @nickdesaulniers -- are you thinking this might be a pedantic warning because
> it's always valid to cast to a pointer to char and write into it? e.g.,
> ```
> short s;
> char *cp = (char *)&s;
> *cp = 0; // Not UB
> ```
No, was just curious about the comment `Is this a clang bug ?`. Looks like my
comment has moved from the source location, and that you have a similar thread
below. This one can be marked done.
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
