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

[Nick Desaulniers via Phabricator via cfe-commits]

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