[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL369791: [Sema] Dont warn on printf(%hd, 
[char]) (PR41467) (authored by Nathan-Huckleberry, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D66186?vs=216893=216904#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  cfe/trunk/lib/AST/FormatString.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/test/FixIt/format.m
  cfe/trunk/test/Sema/format-strings-enum-fixed-type.cpp
  cfe/trunk/test/Sema/format-strings-pedantic.c
  cfe/trunk/test/Sema/format-strings.c


Index: cfe/trunk/test/FixIt/format.m
===
--- cfe/trunk/test/FixIt/format.m
+++ cfe/trunk/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: cfe/trunk/test/Sema/format-strings.c
===
--- cfe/trunk/test/Sema/format-strings.c
+++ cfe/trunk/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: cfe/trunk/test/Sema/format-strings-pedantic.c
===
--- cfe/trunk/test/Sema/format-strings-pedantic.c
+++ cfe/trunk/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: cfe/trunk/test/Sema/format-strings-enum-fixed-type.cpp
===
--- cfe/trunk/test/Sema/format-strings-enum-fixed-type.cpp
+++ cfe/trunk/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -8119,9 +8119,13 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch == analyze_printf::ArgType::Match)
   return true;
+if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+  Pedantic = true;
   }
 }
   } else 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LG, thank you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216893.
Nathan-Huckleberry added a comment.

- Add if without else


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,13 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch == analyze_printf::ArgType::Match)
   return true;
+if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+  Pedantic = true;
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case BuiltinType::SChar:
   case BuiltinType::Char_U:
   case 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Sema/SemaChecking.cpp:8106
   return true;
+Pedantic = true;
   }

`if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)` should have 
stayed probably?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216884.
Nathan-Huckleberry added a comment.

- Remove else


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,12 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch == analyze_printf::ArgType::Match)
   return true;
+Pedantic = true;
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case BuiltinType::SChar:
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
+if (T == C.UnsignedShortTy || T == C.ShortTy)
+  

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

In D66186#1642607 , @aaron.ballman 
wrote:

> LGTM aside from the else after return nit.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM aside from the else after return nit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Looks about right




Comment at: clang/lib/Sema/SemaChecking.cpp:8105-8106
+if (ImplicitMatch == analyze_printf::ArgType::Match)
   return true;
+else if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+  Pedantic = true;

No else after return.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.

Thanks for being responsive to all this code review! 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216739.
Nathan-Huckleberry added a comment.

- Simplify logic for readability


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,13 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch == analyze_printf::ArgType::Match)
   return true;
+else if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+  Pedantic = true;
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case BuiltinType::SChar:
   case BuiltinType::Char_U:

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8107
+return true;
+  Pedantic |= ImplicitMatch == 
analyze_printf::ArgType::NoMatchPedantic;
+}

At this point `ImplicitMatch` can only have the value 
`analyze_printf::ArgType::NoMatchPedantic` as `ArgType::MatchKind` is an enum 
with only 3 values (see `clang/include/clang/AST/FormatString.h`).

Rather than have conditional/ternaries for each case, I think 2 conditionals 
are maybe simpler:

```
if (ImplicitMatch == analyze_printf::ArgType::Match)
  return true;
else if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
  Pedantic = true;
```

The `|=` is kind of more complicated than simple assignment.

Then you don't need a block for `if (ImplicitMatch)`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216730.
Nathan-Huckleberry added a comment.

- Fix broken logic from previous revision


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,14 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
-  return true;
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch) {
+  if (ImplicitMatch == analyze_printf::ArgType::Match)
+return true;
+  Pedantic |= ImplicitMatch == 
analyze_printf::ArgType::NoMatchPedantic;
+}
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

Please can you explain why the snippet i posted in-line does not work for you?




Comment at: clang/lib/Sema/SemaChecking.cpp:8105-8107
+  if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
   return true;

I do not understand this.
Even if you set `Pedantic = true`, you will immediately `return true` without 
ever using that `Pedantic`.
This seems broken?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216729.
Nathan-Huckleberry added a comment.

- Remove else and |=


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,14 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch) {
+  if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
   return true;
+}
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case BuiltinType::SChar:
   case BuiltinType::Char_U:
   case BuiltinType::UChar:
+ 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry marked an inline comment as done.
Nathan-Huckleberry added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;

lebedev.ri wrote:
> lebedev.ri wrote:
> > Just add a new variable
> > ```
> > // All further checking is done on the subexpression
> > analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, 
> > ExprTy);
> > if (Match2 == analyze_printf::ArgType::Match)
> >   return true;
> > Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;
> > ```
> Early return would simplify this still
Could be ArgType::NoMatch and wouldn't display a warning


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry marked an inline comment as done.
Nathan-Huckleberry added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;

Nathan-Huckleberry wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > Just add a new variable
> > > ```
> > > // All further checking is done on the subexpression
> > > analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, 
> > > ExprTy);
> > > if (Match2 == analyze_printf::ArgType::Match)
> > >   return true;
> > > Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;
> > > ```
> > Early return would simplify this still
> Could be ArgType::NoMatch and wouldn't display a warning
Wait this should definitely be an else-if


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry marked an inline comment as done.
Nathan-Huckleberry added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;

Nathan-Huckleberry wrote:
> Nathan-Huckleberry wrote:
> > lebedev.ri wrote:
> > > lebedev.ri wrote:
> > > > Just add a new variable
> > > > ```
> > > > // All further checking is done on the subexpression
> > > > analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, 
> > > > ExprTy);
> > > > if (Match2 == analyze_printf::ArgType::Match)
> > > >   return true;
> > > > Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;
> > > > ```
> > > Early return would simplify this still
> > Could be ArgType::NoMatch and wouldn't display a warning
> Wait this should definitely be an else-if
Nevermind you are correct. Will remove else case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8106
+  if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic |= true;
+  else

I don't think this needs to use `|= true`. If `Pedantic` was true, this is a 
noop. If it was false, this sets it to true. Either way the value is true, so I 
think it should just be `Pedantic = true;` The logic gets easier if you write 
this as:
```
if (ImplicitMatch && ImplicitMatch != analyze_printf::ArgType::NoMatchPedantic)
  return true;
Pedantic |= ImplicitMatch;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;

lebedev.ri wrote:
> Just add a new variable
> ```
> // All further checking is done on the subexpression
> analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, ExprTy);
> if (Match2 == analyze_printf::ArgType::Match)
>   return true;
> Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;
> ```
Early return would simplify this still


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216713.
Nathan-Huckleberry added a comment.

- Add variable for implicit match and fix comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c


Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // warning with -Wformat-pedantic only
+  printf("%hu\n", (uint8_t)1);   // warning with -Wformat-pedantic only
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem 
%S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned 
char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -79,10 +79,10 @@
   printf("%hhd", input); // no-warning
   printf("%hhd", CharConstant); // no-warning
 
-  // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but 
the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 
'short'}}
-  
+  // This is not correct, but it is safe. Only warned in pedantic mode because 
'%hd' shows intent.
+  printf("%hd", input);
+  printf("%hd", CharConstant);
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 
'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 
'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8098,9 +8098,15 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
-  return true;
+// All further checking is done on the subexpression
+const analyze_printf::ArgType::MatchKind ImplicitMatch =
+AT.matchesType(S.Context, ExprTy);
+if (ImplicitMatch) {
+  if (ImplicitMatch == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic |= true;
+  else
+return true;
+}
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- clang/lib/AST/FormatString.cpp
+++ clang/lib/AST/FormatString.cpp
@@ -386,6 +386,8 @@
   case BuiltinType::SChar:
  

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8101
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {

Maybe leave the top level Match const and just create a new one?  It may be 
surprising if someone goes to reuse `Match` below not noticing that it may be 
modified here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/test/Sema/format-strings-enum-fixed-type.cpp:82
 
   // This is not correct but it is safe. We warn because '%hd' shows intent.
+  printf("%hd", input);// no-warning

aaron.ballman wrote:
> This comment is now incorrect.
Not quite what I had in mind. I would remove the `// no-warning` comments that 
were added and instead change the comment on line 82 to say `This is not 
correct, but it is safe. Only warned in pedantic mode because '%hd' shows 
intent.` or something along those lines.



Comment at: clang/test/Sema/format-strings.c:280-281
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 
'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies 
type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 
'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // no-warning
+  printf("%hu\n", (uint8_t)1);   // no-warning
 }

I'd drop the `no-warning` comments here, or say `warning with -Wformat-pedantic 
only` if you think it adds value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:8080-8083
+  analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
   bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
   if (Match == analyze_printf::ArgType::Match)
 return true;

`Match` isn't used outside of this block later on, so i don't think you need 
*this* change.



Comment at: clang/lib/Sema/SemaChecking.cpp:8100-8107
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;

Just add a new variable
```
// All further checking is done on the subexpression
analyze_printf::ArgType::MatchKind Match2 = AT.matchesType(S.Context, ExprTy);
if (Match2 == analyze_printf::ArgType::Match)
  return true;
Pedantic |= Match2 == analyze_printf::ArgType::NoMatchPedantic;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216702.
Nathan-Huckleberry added a comment.

- Cleanup test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c

Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // no-warning
+  printf("%hu\n", (uint8_t)1);   // no-warning
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem %S/Inputs %s
+
+int printf(const char *restrict, ...);
+
+typedef unsigned char uint8_t;
+
+void print_char_as_short() {
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -80,9 +80,9 @@
   printf("%hhd", CharConstant); // no-warning
 
   // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 'short'}}
-  
+  printf("%hd", input);// no-warning
+  printf("%hd", CharConstant); // no-warning
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8077,8 +8077,7 @@
 ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  const analyze_printf::ArgType::MatchKind Match =
-  AT.matchesType(S.Context, ExprTy);
+  analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
   bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
   if (Match == analyze_printf::ArgType::Match)
 return true;
@@ -8098,9 +8097,14 @@
   // function.
   if (ICE->getType() == S.Context.IntTy ||
   ICE->getType() == S.Context.UnsignedIntTy) {
-// All further checking is done on the subexpression.
-if (AT.matchesType(S.Context, ExprTy))
-  return true;
+// All further checking is done on the subexpression
+Match = AT.matchesType(S.Context, ExprTy);
+if (Match) {
+  if (Match == analyze_printf::ArgType::NoMatchPedantic)
+Pedantic = true;
+  else
+return true;
+}
   }
 }
   } else if (const CharacterLiteral *CL = dyn_cast(E)) {
Index: clang/lib/AST/FormatString.cpp
===
--- 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry updated this revision to Diff 216699.
Nathan-Huckleberry added a comment.

- Warn when -Wformat-pedantic is set


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186

Files:
  clang/lib/AST/FormatString.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/FixIt/format.m
  clang/test/Sema/format-strings-enum-fixed-type.cpp
  clang/test/Sema/format-strings-pedantic.c
  clang/test/Sema/format-strings.c

Index: clang/test/Sema/format-strings.c
===
--- clang/test/Sema/format-strings.c
+++ clang/test/Sema/format-strings.c
@@ -277,8 +277,8 @@
 
 void should_understand_small_integers() {
   printf("%hhu", (short) 10); // expected-warning{{format specifies type 'unsigned char' but the argument has type 'short'}}
-  printf("%hu\n", (unsigned char) 1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
-  printf("%hu\n", (uint8_t)1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'uint8_t'}}
+  printf("%hu\n", (unsigned char)1); // no-warning
+  printf("%hu\n", (uint8_t)1);   // no-warning
 }
 
 void test11(void *p, char *s) {
Index: clang/test/Sema/format-strings-pedantic.c
===
--- /dev/null
+++ clang/test/Sema/format-strings-pedantic.c
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wformat -Wformat-pedantic -isystem %S/Inputs %s
+
+#include 
+#include 
+#define __need_wint_t
+#include  // For wint_t and wchar_t
+
+typedef struct _FILE FILE;
+int fprintf(FILE *, const char *restrict, ...);
+int printf(const char *restrict, ...);
+int snprintf(char *restrict, size_t, const char *restrict, ...);
+int sprintf(char *restrict, const char *restrict, ...);
+int vasprintf(char **, const char *, va_list);
+int asprintf(char **, const char *, ...);
+int vfprintf(FILE *, const char *restrict, va_list);
+int vprintf(const char *restrict, va_list);
+int vsnprintf(char *, size_t, const char *, va_list);
+int vsprintf(char *restrict, const char *restrict, va_list);
+
+int vscanf(const char *restrict format, va_list arg);
+
+typedef unsigned char uint8_t;
+
+void should_understand_small_integers() {
+  printf("%hhu", (short)10); // expected-warning{{format specifies type 'unsigned char' but the argument has type 'short'}}
+  printf("%hu\n", (unsigned char)1); // expected-warning{{format specifies type 'unsigned short' but the argument has type 'unsigned char'}}
+  printf("%hu\n", (uint8_t)1);   // expected-warning{{format specifies type 'unsigned short' but the argument has type 'uint8_t' (aka 'unsigned char')}}
+}
Index: clang/test/Sema/format-strings-enum-fixed-type.cpp
===
--- clang/test/Sema/format-strings-enum-fixed-type.cpp
+++ clang/test/Sema/format-strings-enum-fixed-type.cpp
@@ -80,9 +80,9 @@
   printf("%hhd", CharConstant); // no-warning
 
   // This is not correct but it is safe. We warn because '%hd' shows intent.
-  printf("%hd", input); // expected-warning{{format specifies type 'short' but the argument has underlying type 'char'}}
-  printf("%hd", CharConstant); // expected-warning{{format specifies type 'short'}}
-  
+  printf("%hd", input);// no-warning
+  printf("%hd", CharConstant); // no-warning
+
   // This is not correct but it matches the promotion rules (and is safe).
   printf("%d", input); // no-warning
   printf("%d", CharConstant); // no-warning
Index: clang/test/FixIt/format.m
===
--- clang/test/FixIt/format.m
+++ clang/test/FixIt/format.m
@@ -205,9 +205,7 @@
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%f"
   // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:16}:"(unichar)"
 
-  NSLog(@"%C", (char)0x260300); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
-  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
-  // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-2]]:16-[[@LINE-2]]:22}:"(unichar)"
+  NSLog(@"%C", (char)0x260300);
 
   NSLog(@"%C", 'a'); // expected-warning{{format specifies type 'unichar' (aka 'unsigned short') but the argument has type 'char'}}
   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:11-[[@LINE-1]]:13}:"%c"
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -8077,8 +8077,7 @@
 ExprTy = TET->getUnderlyingExpr()->getType();
   }
 
-  const analyze_printf::ArgType::MatchKind Match =
-  AT.matchesType(S.Context, ExprTy);
+  analyze_printf::ArgType::MatchKind Match = AT.matchesType(S.Context, ExprTy);
   bool Pedantic = Match == analyze_printf::ArgType::NoMatchPedantic;
   if (Match == 

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a comment.

In D66186#1632440 , 
@Nathan-Huckleberry wrote:

> As far as I can tell this case was just overlooked. The original commit 
> adding this change 
> https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 only 
> allows chars to int and chars to chars. Another commit ignores typing of 
> chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I 
> did not see anything related to this particular case in previous commits.


Hmm, it looks like, at least from this review, someone thought the behavior was 
for demonstrating user intent: 
http://llvm.org/viewvc/llvm-project?view=revision=157961.

I've convinced myself that -Wformat should disable that diagnostic by default, 
but there is utility in keeping it exposed through a different format warning 
flag. It seems like `-Wformat-pedantic` should still diagnose this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry added a comment.

As far as I can tell this case was just overlooked. The original commit adding 
this change https://reviews.llvm.org/rG0208793e41018ac168412a3da8b2fba70aba9716 
only allows chars to int and chars to chars. Another commit ignores typing of 
chars https://reviews.llvm.org/rG74e82bd190017d59d5d78b07dedca5b06b4547da. I 
did not see anything related to this particular case in previous commits.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
This revision now requires changes to proceed.

(just want to mark it as "unanswered questions")


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D66186#1631921 , 
@Nathan-Huckleberry wrote:

> In D66186#1630427 , @aaron.ballman 
> wrote:
>
> > There was a request in the linked bug for some code archaeology to see why 
> > this behavior exists in the first place. What were the results of that? I'm 
> > not opposed to the patch, but I would like to understand why it behaves the 
> > way it does.
>
>
> Since printf is a variadic function, integral argument types are promoted to 
> int. The warning code runs the matchesType check twice, once to check if the 
> promoted type (int) is able to be printed with the format and once to check 
> if the original type (char) is able to be printed with the format.
>
> `printf("%d", [char])` is caught by the first case
>  `printf("%hhd", [char])` is caught by the second case.
>
> `printf("%hd", [char])` is a warning because an exception has not been made 
> for that case.


This all makes sense as to how things work today, but I was more wondering why 
they worked that way in the first place. I'm especially interested to know 
whether this is diagnosed because it shows confusion of the user's intent, 
because that seems like a valuable behavior to retain (though perhaps it 
doesn't need to be default-on).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D66186#1631921 , 
@Nathan-Huckleberry wrote:

> In D66186#1630427 , @aaron.ballman 
> wrote:
>
> > There was a request in the linked bug for some code archaeology to see why 
> > this behavior exists in the first place. What were the results of that? I'm 
> > not opposed to the patch, but I would like to understand why it behaves the 
> > way it does.
>
>
> Since printf is a variadic function, integral argument types are promoted to 
> int. The warning code runs the matchesType check twice, once to check if the 
> promoted type (int) is able to be printed with the format and once to check 
> if the original type (char) is able to be printed with the format.
>
> `printf("%d", [char])` is caught by the first case
>  `printf("%hhd", [char])` is caught by the second case.
>
> `printf("%hd", [char])` is a warning because an exception has not been made 
> for that case.


That explains what the implementation does, but does not attempt to answer the 
question *why* things are the way they are.

I read https://bugs.llvm.org/show_bug.cgi?id=41467#c4 as

- any narrowing is always diagnosed
- promotion to wider than int is diagnosed
- passthrough is not diagnosed
- promotion to something smaller than int is diagnosed (the current case)

I can interpret it as: we already know that

In D66186#1631921 , 
@Nathan-Huckleberry wrote:

> Since printf is a variadic function, integral argument types are promoted to 
> int.


therefore why are you first implicitly promoting to int and then implicitly 
truncating?
Did you mean to print the original value? Did you mean to print int?

That doesn't sound too outlandish to me.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Nathan Huckleberry via Phabricator via cfe-commits]

Nathan-Huckleberry added a comment.

In D66186#1630427 , @aaron.ballman 
wrote:

> There was a request in the linked bug for some code archaeology to see why 
> this behavior exists in the first place. What were the results of that? I'm 
> not opposed to the patch, but I would like to understand why it behaves the 
> way it does.


Since printf is a variadic function, integral argument types are promoted to 
int. The warning code runs the matchesType check twice, once to check if the 
promoted type (int) is able to be printed with the format and once to check if 
the original type (char) is able to be printed with the format.

`printf("%d", [char])` is caught by the first case
`printf("%hhd", [char])` is caught by the second case.

`printf("%hd", [char])` is a warning because an exception has not been made for 
that case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

There was a request in the linked bug for some code archaeology to see why this 
behavior exists in the first place. What were the results of that? I'm not 
opposed to the patch, but I would like to understand why it behaves the way it 
does.

I could imagine "confusing user intent" being a valid reason why someone might 
want this warning, so we may want to default-off this diagnostic (because the 
code is safe) but still provide users with a way to enable it.




Comment at: clang/test/Sema/format-strings-enum-fixed-type.cpp:82
 
   // This is not correct but it is safe. We warn because '%hd' shows intent.
+  printf("%hd", input);// no-warning

This comment is now incorrect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66186/new/

https://reviews.llvm.org/D66186



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits