[clang] Fix message text for `-Wpointer-sign` to account for plain char

[hubert.reinterpretcast]

Summary

The -Wpointer-sign warning text is inappropriate for describing the incompatible pointer conversion between plain char and explicitly signed/unsigned char (whichever plain char has the same range as) and vice versa.

Specifically, in part, it reads "converts between pointers to integer types with different sign". This patch changes that portion to read instead as "converts between pointers to integer types where one is of the unique plain char type and the other is not" when one of the types is plain char.

C17 subclause 6.5.16.1 indicates that the conversions resulting in -Wpointer-sign warnings in assignment-like contexts are constraint violations. This means that strict conformance requires a diagnostic for the case where the message text is wrong before this patch. The lack of an even more specialized warning group is consistent with GCC.

[hubert.reinterpretcast]

Ping, @aaron.ballman?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7783–7784

I fee like that differ by signed/unsigned/plain variation is a bit hard for users to understand and maybe we want to spell it out a bit more explicitly. I took a stab at a more wordy diagnostic that I think is easier to understand, but if you have other ideas, I'm not tied to my wording. WDYT?

(Same suggestion applies below -- though we may want to switch to ext_typecheck_convert_incompatible_pointer_sign.Text below rather than spelling all this out manually.)

[hubert.reinterpretcast]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7783–7784

I think the "other excludes sign information" wording makes this sound like a portability diagnostic. I was going for wording that retains the same "seriousness" for the plain char versus signed/unsigned char case as for the signed char versus unsigned char case.

Would "where one is of the unique plain char type and the other is not" work?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

7783–7784

Would "where one is of the unique plain char type and the other is not" work?

Yeah, I think that's great wording, thank you!

[hubert.reinterpretcast]

• Address review: Mention plain char only when it appears

[aaron.ballman]

LGTM aside from some very minor nits.

clang/include/clang/Basic/DiagnosticSemaKinds.td

7784

clang/lib/Sema/SemaExpr.cpp

15967 ↗

(On Diff #315648)

We don't typically use top-level const on value types for local variables.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def ext_typecheck_convert_incompatible_pointer_sign : ExtWarn<
   "|%diff{returning $ from a function with result type $|"
   "returning from function with different return type}0,1"
   "|%diff{converting $ to type $|converting between types}0,1"
   "|%diff{initializing $ with an expression of type $|"
   "initializing with expression of different type}0,1"
   "|%diff{sending $ to parameter of type $|"
   "sending to parameter of different type}0,1"
   "|%diff{casting $ to type $|casting between types}0,1}2"
-  " converts between pointers to integer types with different sign">,
+  " converts between pointers to integer types that differ by"
+  " signed/unsigned/plain variation">,
   InGroup<DiagGroup<"pointer-sign">>;
 def err_typecheck_convert_incompatible_pointer_sign : Error<
   "%select{%diff{assigning to $ from $|assigning to different types}0,1"
   "|%diff{passing $ to parameter of type $|"
   "passing to parameter of different type}0,1"
   "|%diff{returning $ from a function with result type $|"
   "returning from function with different return type}0,1"
   "|%diff{converting $ to type $|converting between types}0,1"
   "|%diff{initializing $ with an expression of type $|"
   "initializing with expression of different type}0,1"
   "|%diff{sending $ to parameter of type $|"
   "sending to parameter of different type}0,1"
   "|%diff{casting $ to type $|casting between types}0,1}2"
-  " converts between pointers to integer types with different sign">;
+  " converts between pointers to integer types that differ by"
+  " signed/unsigned/plain variation">;
 def ext_typecheck_convert_incompatible_pointer : ExtWarn<
   "incompatible pointer types "
   "%select{%diff{assigning to $ from $|assigning to different types}0,1"
   "|%diff{passing $ to parameter of type $|"
   "passing to parameter of different type}0,1"
   "|%diff{returning $ from a function with result type $|"
   "returning from function with different return type}0,1"
   "|%diff{converting $ to type $|converting between types}0,1"

clang/test/Sema/incompatible-sign.c

 // RUN: %clang_cc1 %s -verify -fsyntax-only
+// RUN: %clang_cc1 %s -verify -fsyntax-only -fno-signed-char
 
 int a(int* x); // expected-note{{passing argument to parameter 'x' here}}
-int b(unsigned* y) { return a(y); } // expected-warning {{passing 'unsigned int *' to parameter of type 'int *' converts between pointers to integer types with different sign}}
+int b(unsigned* y) { return a(y); } // expected-warning {{passing 'unsigned int *' to parameter of type 'int *' converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 
+signed char *plainCharToSignedChar(signed char *arg) { // expected-note{{passing argument to parameter}}
+  extern char c;
+  signed char *p = &c; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  struct { signed char *p; } s = { &c }; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  p = &c; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  plainCharToSignedChar(&c); // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  return &c; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+}
+
+char *unsignedCharToPlainChar(char *arg) { // expected-note{{passing argument to parameter}}
+  extern unsigned char uc;
+  char *p = &uc; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  (void) (char *[]){ [42] = &uc }; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  p = &uc; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  unsignedCharToPlainChar(&uc); // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+  return &uc; // expected-warning {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+}

clang/test/Sema/incompatible-sign.cpp

+// RUN: %clang_cc1 %s -verify -fsyntax-only
+// RUN: %clang_cc1 %s -verify -fsyntax-only -fno-signed-char
+
+void plainToSigned() {
+  extern char c;
+  signed char *p;
+  p = &c; // expected-error {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+}
+
+void unsignedToPlain() {
+  extern unsigned char uc;
+  char *p;
+  p = &uc; // expected-error {{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
+}

clang/test/Sema/vector-ops.c

void test1(v2u v2ua, v2s v2sa, v2f v2fa) {
   // Arrays
   int array1[v2ua]; // expected-error{{size of array has non-integer type 'v2u' (vector of 2 'unsigned int' values}}
   int array2[17];
   // FIXME: error message below needs type!
   (void)(array2[v2ua]); // expected-error{{array subscript is not an integer}}
 
   v2u *v2u_ptr = 0;
   v2s *v2s_ptr;
-  v2s_ptr = v2u_ptr; // expected-warning{{converts between pointers to integer types with different sign}}
+  v2s_ptr = v2u_ptr; // expected-warning{{converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }
 
 void testLogicalVecVec(v2u v2ua, v2s v2sa, v2f v2fa) {
   // Logical operators
   v2ua = v2ua && v2ua; // expected-error {{logical expression with vector types 'v2u' (vector of 2 'unsigned int' values) and 'v2u'}}
   v2ua = v2ua || v2ua; // expected-error {{logical expression with vector types 'v2u' (vector of 2 'unsigned int' values) and 'v2u'}}
 
   v2ua = v2sa && v2ua; // expected-error {{logical expression with vector types 'v2s' (vector of 2 'int' values) and 'v2u' (vector of 2 'unsigned int' values)}}

clang/test/SemaObjC/objc-cf-audited-warning.m

 const CFAllocatorRef kCFAllocatorDefault;
 
 CF_IMPLICIT_BRIDGING_ENABLED
 CFURLRef CFURLCreateFromFileSystemRepresentation(CFAllocatorRef allocator, const UInt8 *buffer, CFIndex bufLen, Boolean isDirectory); // expected-note {{passing argument to parameter 'buffer' here}}
 CF_IMPLICIT_BRIDGING_DISABLED
 
 void saveImageToJPG(const char *filename)
 {
-    CFURLRef url = CFURLCreateFromFileSystemRepresentation(kCFAllocatorDefault, filename, 10, 0); // expected-warning {{passing 'const char *' to parameter of type 'const UInt8 *' (aka 'const unsigned char *') converts between pointers to integer types with different sign}}
+    CFURLRef url = CFURLCreateFromFileSystemRepresentation(kCFAllocatorDefault, filename, 10, 0); // expected-warning {{passing 'const char *' to parameter of type 'const UInt8 *' (aka 'const unsigned char *') converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }

clang/test/SemaOpenCL/builtins-amdgcn-error.cl

void test_atomic_inc32() {
   val = __builtin_amdgcn_atomic_inc32(&val, val, __ATOMIC_ACQUIRE - 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_inc32(4);                                            // expected-error {{too few arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_inc32(&val, val, 4, 4, 4, 4);                        // expected-error {{too many arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_inc32(&val, val, 3.14, "");                          // expected-warning {{implicit conversion from 'double' to 'unsigned int' changes value from 3.14 to 3}}
   val = __builtin_amdgcn_atomic_inc32(&val, val, __ATOMIC_ACQUIRE, 5);               // expected-warning {{incompatible integer to pointer conversion passing 'int' to parameter of type 'const char *'}}
   const char ptr[] = "workgroup";
   val = __builtin_amdgcn_atomic_inc32(&val, val, __ATOMIC_ACQUIRE, ptr); // expected-error {{expression is not a string literal}}
   int signedVal = 15;
-  signedVal = __builtin_amdgcn_atomic_inc32(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private int *' to parameter of type 'volatile __private unsigned int *' converts between pointers to integer types with different sign}}
+  signedVal = __builtin_amdgcn_atomic_inc32(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private int *' to parameter of type 'volatile __private unsigned int *' converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }
 
 void test_atomic_inc64() {
   __UINT64_TYPE__ val = 17;
   val = __builtin_amdgcn_atomic_inc64(&val, val, __ATOMIC_SEQ_CST + 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_inc64(&val, val, __ATOMIC_ACQUIRE - 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_inc64(4);                                            // expected-error {{too few arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_inc64(&val, val, 4, 4, 4, 4);                        // expected-error {{too many arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_inc64(&val, val, 3.14, "");                          // expected-warning {{implicit conversion from 'double' to 'unsigned int' changes value from 3.14 to 3}}
   val = __builtin_amdgcn_atomic_inc64(&val, val, __ATOMIC_ACQUIRE, 5);               // expected-warning {{incompatible integer to pointer conversion passing 'int' to parameter of type 'const char *'}}
   const char ptr[] = "workgroup";
   val = __builtin_amdgcn_atomic_inc64(&val, val, __ATOMIC_ACQUIRE, ptr); // expected-error {{expression is not a string literal}}
   __INT64_TYPE__ signedVal = 15;
-  signedVal = __builtin_amdgcn_atomic_inc64(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private long *' to parameter of type 'volatile __private unsigned long *' converts between pointers to integer types with different sign}}
+  signedVal = __builtin_amdgcn_atomic_inc64(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private long *' to parameter of type 'volatile __private unsigned long *' converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }
 
 void test_atomic_dec32() {
   uint val = 17;
   val = __builtin_amdgcn_atomic_dec32(&val, val, __ATOMIC_SEQ_CST + 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_dec32(&val, val, __ATOMIC_ACQUIRE - 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_dec32(4);                                            // expected-error {{too few arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_dec32(&val, val, 4, 4, 4, 4);                        // expected-error {{too many arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_dec32(&val, val, 3.14, "");                          // expected-warning {{implicit conversion from 'double' to 'unsigned int' changes value from 3.14 to 3}}
   val = __builtin_amdgcn_atomic_dec32(&val, val, __ATOMIC_ACQUIRE, 5);               // expected-warning {{incompatible integer to pointer conversion passing 'int' to parameter of type 'const char *'}}
   const char ptr[] = "workgroup";
   val = __builtin_amdgcn_atomic_dec32(&val, val, __ATOMIC_ACQUIRE, ptr); // expected-error {{expression is not a string literal}}
   int signedVal = 15;
-  signedVal = __builtin_amdgcn_atomic_dec32(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private int *' to parameter of type 'volatile __private unsigned int *' converts between pointers to integer types with different sign}}
+  signedVal = __builtin_amdgcn_atomic_dec32(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private int *' to parameter of type 'volatile __private unsigned int *' converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }
 
 void test_atomic_dec64() {
   __UINT64_TYPE__ val = 17;
   val = __builtin_amdgcn_atomic_dec64(&val, val, __ATOMIC_SEQ_CST + 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_dec64(&val, val, __ATOMIC_ACQUIRE - 1, "workgroup"); // expected-warning {{memory order argument to atomic operation is invalid}}
   val = __builtin_amdgcn_atomic_dec64(4);                                            // expected-error {{too few arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_dec64(&val, val, 4, 4, 4, 4);                        // expected-error {{too many arguments to function call, expected 4}}
   val = __builtin_amdgcn_atomic_dec64(&val, val, 3.14, "");                          // expected-warning {{implicit conversion from 'double' to 'unsigned int' changes value from 3.14 to 3}}
   val = __builtin_amdgcn_atomic_dec64(&val, val, __ATOMIC_ACQUIRE, 5);               // expected-warning {{incompatible integer to pointer conversion passing 'int' to parameter of type 'const char *'}}
   const char ptr[] = "workgroup";
   val = __builtin_amdgcn_atomic_dec64(&val, val, __ATOMIC_ACQUIRE, ptr); // expected-error {{expression is not a string literal}}
   __INT64_TYPE__ signedVal = 15;
-  signedVal = __builtin_amdgcn_atomic_dec64(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private long *' to parameter of type 'volatile __private unsigned long *' converts between pointers to integer types with different sign}}
+  signedVal = __builtin_amdgcn_atomic_dec64(&signedVal, signedVal, __ATOMIC_ACQUIRE, ""); // expected-warning {{passing '__private long *' to parameter of type 'volatile __private unsigned long *' converts between pointers to integer types that differ by signed/unsigned/plain variation}}
 }