[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

7817–7819

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

7817–7819

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

7817–7819

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

7818

clang/lib/Sema/SemaExpr.cpp

15967

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 %select{with different sign|"
+  "where one is of the unique plain 'char' type and the other is not}3">,
   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">;
+def err_typecheck_convert_incompatible_pointer_sign :
+  Error<ext_typecheck_convert_incompatible_pointer_sign.Text>;
 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/lib/Sema/SemaExpr.cpp

bool Sema::DiagnoseAssignmentResult(AssignConvertType ConvTy,
   }
 
   PartialDiagnostic FDiag = PDiag(DiagKind);
   if (Action == AA_Passing_CFAudited)
     FDiag << FirstType << SecondType << AA_Passing << SrcExpr->getSourceRange();
   else
     FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
 
+  if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
+      DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {
+    auto isPlainChar = [](const clang::Type *Type) {
+      return Type->isSpecificBuiltinType(BuiltinType::Char_S) ||
+             Type->isSpecificBuiltinType(BuiltinType::Char_U);
+    };
+    FDiag << (isPlainChar(FirstType->getPointeeOrArrayElementType()) ||
+              isPlainChar(SecondType->getPointeeOrArrayElementType()));
+  }
+
   // If we can fix the conversion, suggest the FixIts.
   if (!ConvHints.isNull()) {
     for (FixItHint &H : ConvHints.Hints)
       FDiag << H;
   }
 
   if (MayHaveConvFixit) { FDiag << (unsigned) (ConvHints.Kind); }
 

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}}
 
 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 with different sign}}
-  struct { signed char *p; } s = { &c }; // expected-warning {{converts between pointers to integer types with different sign}}
-  p = &c; // expected-warning {{converts between pointers to integer types with different sign}}
-  plainCharToSignedChar(&c); // expected-warning {{converts between pointers to integer types with different sign}}
-  return &c; // expected-warning {{converts between pointers to integer types with different sign}}
+  signed char *p = &c; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  struct { signed char *p; } s = { &c }; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  p = &c; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  plainCharToSignedChar(&c); // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  return &c; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
 }
 
 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 with different sign}}
-  (void) (char *[]){ [42] = uc }; // expected-warning {{converts between pointers to integer types with different sign}}
-  p = uc; // expected-warning {{converts between pointers to integer types with different sign}}
-  unsignedCharToPlainChar(uc); // expected-warning {{converts between pointers to integer types with different sign}}
-  return uc; // expected-warning {{converts between pointers to integer types with different sign}}
+  char *p = uc; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  (void) (char *[]){ [42] = uc }; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  p = uc; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  unsignedCharToPlainChar(uc); // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
+  return uc; // expected-warning {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
 }

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 with different sign}}
+  p = &c; // expected-error {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
 }
 
 void unsignedToPlain() {
   extern unsigned char uc;
   char *p;
-  p = &uc; // expected-error {{converts between pointers to integer types with different sign}}
+  p = &uc; // expected-error {{converts between pointers to integer types where one is of the unique plain 'char' type and the other is not}}
 }

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 where one is of the unique plain 'char' type and the other is not}}
 }