[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
This revision was automatically updated to reflect the committed changes.
Closed by commit rGc6ffe4d76fbf: [clang] Fix message text for `-Wpointer-sign`
to account for plain char (authored by hubert.reinterpretcast).
Changed prior to commit:
https://reviews.llvm.org/D93999?vs=315648&id=315956#toc
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/incompatible-sign.c
clang/test/Sema/incompatible-sign.cpp
clang/test/SemaObjC/objc-cf-audited-warning.m
Index: clang/test/SemaObjC/objc-cf-audited-warning.m
===
--- clang/test/SemaObjC/objc-cf-audited-warning.m
+++ clang/test/SemaObjC/objc-cf-audited-warning.m
@@ -20,5 +20,5 @@
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}}
}
Index: clang/test/Sema/incompatible-sign.cpp
===
--- clang/test/Sema/incompatible-sign.cpp
+++ clang/test/Sema/incompatible-sign.cpp
@@ -4,11 +4,11 @@
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}}
}
Index: clang/test/Sema/incompatible-sign.c
===
--- clang/test/Sema/incompatible-sign.c
+++ clang/test/Sema/incompatible-sign.c
@@ -6,18 +6,18 @@
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
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from some very minor nits.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7784
+ " 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>;
Comment at: clang/lib/Sema/SemaExpr.cpp:15967
+ DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {
+const auto isPlainChar = [](const clang::Type *Type) {
+ return Type->isSpecificBuiltinType(BuiltinType::Char_S) ||
We don't typically use top-level `const` on value types for local variables.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
hubert.reinterpretcast updated this revision to Diff 315648.
hubert.reinterpretcast added a comment.
- Address review: Mention plain char only when it appears
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/Sema/SemaExpr.cpp
clang/test/Sema/incompatible-sign.c
clang/test/Sema/incompatible-sign.cpp
clang/test/SemaObjC/objc-cf-audited-warning.m
Index: clang/test/SemaObjC/objc-cf-audited-warning.m
===
--- clang/test/SemaObjC/objc-cf-audited-warning.m
+++ clang/test/SemaObjC/objc-cf-audited-warning.m
@@ -20,5 +20,5 @@
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}}
}
Index: clang/test/Sema/incompatible-sign.cpp
===
--- /dev/null
+++ clang/test/Sema/incompatible-sign.cpp
@@ -0,0 +1,14 @@
+// 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 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 where one is of the unique plain char type and the other is not}}
+}
Index: clang/test/Sema/incompatible-sign.c
===
--- clang/test/Sema/incompatible-sign.c
+++ clang/test/Sema/incompatible-sign.c
@@ -1,5 +1,23 @@
// 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 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 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}}
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -15962,6 +15962,16 @@
else
FDiag << FirstType << SecondType << Action << SrcExpr->getSourceRange();
+ if (DiagKind == diag::ext_typecheck_convert_incompatible_pointer_sign ||
+ DiagKind == diag::err_typecheck_convert_incompatible_pointer_sign) {
+const auto isPlainChar = [](const clang::Type *Type) {
+ return Type->isSpeci
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
aaron.ballman added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7783-7784
"|%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>;
hubert.reinterpretcast wrote:
> aaron.ballman wrote:
> > 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.)
> 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?
> 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!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
hubert.reinterpretcast added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7783-7784
"|%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>;
aaron.ballman wrote:
> 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.)
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?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
aaron.ballman added inline comments.
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7783-7784
"|%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>;
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.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93999/new/
https://reviews.llvm.org/D93999
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
hubert.reinterpretcast added a comment. Ping, @aaron.ballman? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93999/new/ https://reviews.llvm.org/D93999 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93999: [clang] Fix message text for `-Wpointer-sign` to account for plain char
hubert.reinterpretcast created this revision.
hubert.reinterpretcast added reviewers: aaron.ballman, rsmith.
Herald added subscribers: jfb, jvesely.
hubert.reinterpretcast requested review of this revision.
Herald added a project: clang.
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 that differ by
signed/unsigned/plain variation".
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93999
Files:
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/test/Sema/incompatible-sign.c
clang/test/Sema/incompatible-sign.cpp
clang/test/Sema/vector-ops.c
clang/test/SemaObjC/objc-cf-audited-warning.m
clang/test/SemaOpenCL/builtins-amdgcn-error.cl
Index: clang/test/SemaOpenCL/builtins-amdgcn-error.cl
===
--- clang/test/SemaOpenCL/builtins-amdgcn-error.cl
+++ clang/test/SemaOpenCL/builtins-amdgcn-error.cl
@@ -156,7 +156,7 @@
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() {
@@ -170,7 +170,7 @@
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() {
@@ -184,7 +184,7 @@
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() {
@@ -198,5 +198,5 @@
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}}
}
Index: clang/test/SemaObjC/objc-cf-audited-warning.m
===
--- clang/test/SemaObjC/objc-cf-audited-warning.m
+++ clang/test/SemaObjC/objc-cf-audi
