One more fix for P3144R2 implementation #149406
Conversation
P3144R2 made it ill-formed to delete a pointer to an incomplete class type, and asserted that incomplete class types were the only incomplete types that were possible to pass to the delete operator. This assertion was wrong, and PR 99278 already updated the check to exclude incomplete enum types, but that is still wrong: pointers to arrays of unknown length are another instance of incomplete types that are possible to delete and were not included in PR3144R2's ban. Additionally, the diagnostic still claimed that the problem was with deleting pointers to incomplete types, rather than to incomplete class types. This PR ensures that when we implement the check for incomplete class type, we only check for incomplete class type, and adjusts the diagnostics to say 'incomplete struct' or 'incomplete union' to be accurate without being overly verbose.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 Author: Harald van Dijk (hvdijk) ChangesP3144R2 made it ill-formed to delete a pointer to an incomplete class type, and asserted that incomplete class types were the only incomplete types that were possible to pass to the delete operator. This assertion was wrong, and PR 99278 already updated the check to exclude incomplete enum types, but that is still wrong: pointers to arrays of unknown length are another instance of incomplete types that are possible to delete and were not included in PR3144R2's ban. Additionally, the diagnostic still claimed that the problem was with deleting pointers to incomplete types, rather than to incomplete class types. This PR ensures that when we implement the check for incomplete class type, we only check for incomplete class type, and adjusts the diagnostics to say 'incomplete struct' or 'incomplete union' to be accurate without being overly verbose. Full diff: https://github.com/llvm/llvm-project/pull/149406.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b2ea65ae111be..d86f10da17c61 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8392,17 +8392,19 @@ def ext_default_init_const : ExtWarn<
"is a Microsoft extension">,
InGroup<MicrosoftConstInit>;
def err_delete_operand : Error<"cannot delete expression of type %0">;
+def err_delete_void_ptr_operand : Error<
+ "cannot delete expression with pointer-to-'void' type %0">;
def ext_delete_void_ptr_operand : ExtWarn<
"cannot delete expression with pointer-to-'void' type %0">,
InGroup<DeleteIncomplete>;
def err_ambiguous_delete_operand : Error<
"ambiguous conversion of delete expression of type %0 to a pointer">;
def warn_delete_incomplete : Warning<
- "deleting pointer to incomplete type %0 is incompatible with C++2c"
+ "deleting pointer to incomplete %select{struct|union}0 %1 is incompatible with C++2c"
" and may cause undefined behavior">,
InGroup<DeleteIncomplete>;
def err_delete_incomplete : Error<
- "cannot delete pointer to incomplete type %0">;
+ "cannot delete pointer to incomplete %select{struct|union}0 %1">;
def err_delete_incomplete_class_type : Error<
"deleting incomplete class type %0; no conversions to pointer type">;
def err_delete_explicit_conversion : Error<
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index f851c9e1d5015..a741acc8a0a89 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4033,25 +4033,24 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
// effectively bans deletion of "void*". However, most compilers support
// this, so we treat it as a warning unless we're in a SFINAE context.
// But we still prohibit this since C++26.
- Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_incomplete
+ Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_void_ptr_operand
: diag::ext_delete_void_ptr_operand)
- << (LangOpts.CPlusPlus26 ? Pointee : Type)
- << Ex.get()->getSourceRange();
+ << Type << Ex.get()->getSourceRange();
} else if (Pointee->isFunctionType() || Pointee->isVoidType() ||
Pointee->isSizelessType()) {
return ExprError(Diag(StartLoc, diag::err_delete_operand)
- << Type << Ex.get()->getSourceRange());
+ << Type << Ex.get()->getSourceRange());
} else if (!Pointee->isDependentType()) {
// FIXME: This can result in errors if the definition was imported from a
// module but is hidden.
- if (Pointee->isEnumeralType() ||
- !RequireCompleteType(StartLoc, Pointee,
- LangOpts.CPlusPlus26
- ? diag::err_delete_incomplete
- : diag::warn_delete_incomplete,
- Ex.get())) {
- if (const RecordType *RT = PointeeElem->getAs<RecordType>())
+ if (const RecordType *RT = PointeeElem->getAs<RecordType>()) {
+ if (!RequireCompleteType(StartLoc, Pointee,
+ LangOpts.CPlusPlus26
+ ? diag::err_delete_incomplete
+ : diag::warn_delete_incomplete,
+ RT->isUnionType(), Ex.get())) {
PointeeRD = cast<CXXRecordDecl>(RT->getDecl());
+ }
}
}
diff --git a/clang/test/Analysis/dtor.cpp b/clang/test/Analysis/dtor.cpp
index c17c886d97fb4..66f555cd65240 100644
--- a/clang/test/Analysis/dtor.cpp
+++ b/clang/test/Analysis/dtor.cpp
@@ -499,7 +499,7 @@ namespace PseudoDtor {
namespace Incomplete {
class Foo; // expected-note{{forward declaration}}
- void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete type}}
+ void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete struct 'Foo'}}
}
namespace TypeTraitExpr {
diff --git a/clang/test/CXX/drs/cwg5xx.cpp b/clang/test/CXX/drs/cwg5xx.cpp
index 1d505adecfb27..269397a0dc221 100644
--- a/clang/test/CXX/drs/cwg5xx.cpp
+++ b/clang/test/CXX/drs/cwg5xx.cpp
@@ -907,7 +907,7 @@ namespace cwg573 { // cwg573: no
// cxx98-error@-1 {{cast between pointer-to-function and pointer-to-object is an extension}}
void f() { delete a; }
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
int n = d - a;
// expected-error@-1 {{arithmetic on pointers to void}}
// FIXME: This is ill-formed.
@@ -1298,12 +1298,12 @@ namespace cwg599 { // cwg599: partial
void f(void *p, void (*q)(), S s, T t, U u, V v) {
delete p;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete q;
// expected-error@-1 {{cannot delete expression of type 'void (*)()'}}
delete s;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete t;
// expected-error@-1 {{cannot delete expression of type 'T'}}
// FIXME: This is valid, but is rejected due to a non-conforming GNU
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp b/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
index ecb29189af60b..4e68fbab5d670 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
@@ -6,12 +6,12 @@
// The trivial case.
class T0; // expected-note {{forward declaration}}
-void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete type}}
+void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete struct 'T0'}}
class T0 { ~T0(); };
// The trivial case, inside a template instantiation.
template<typename T>
-struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete type}}
+struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete struct 'T1_B'}}
class T1_B; // expected-note {{forward declaration}}
void f0() { T1_A<T1_B> x; } // expected-note {{in instantiation of member function}}
@@ -44,3 +44,8 @@ class T3_A {
private:
~T3_A(); // expected-note{{declared private here}}
};
+
+// The trivial case but with a union.
+union T4; // expected-note {{forward declaration}}
+void f4(T4 *a) { delete a; } // expected-warning {{deleting pointer to incomplete union 'T4'}}
+union T4 { ~T4(); };
diff --git a/clang/test/OpenMP/deferred-diags.cpp b/clang/test/OpenMP/deferred-diags.cpp
index e31b99b8c88e4..a978465322e9d 100644
--- a/clang/test/OpenMP/deferred-diags.cpp
+++ b/clang/test/OpenMP/deferred-diags.cpp
@@ -41,7 +41,7 @@ namespace TestDeleteIncompleteClassDefinition {
struct a;
struct b {
b() {
- delete c; // expected-warning {{deleting pointer to incomplete type 'a' is incompatible with C++2c and may cause undefined behavior}}
+ delete c; // expected-warning {{deleting pointer to incomplete struct 'a' is incompatible with C++2c and may cause undefined behavior}}
}
a *c;
};
diff --git a/clang/test/SemaCXX/new-delete.cpp b/clang/test/SemaCXX/new-delete.cpp
index f918501554f80..735908c5dc0bb 100644
--- a/clang/test/SemaCXX/new-delete.cpp
+++ b/clang/test/SemaCXX/new-delete.cpp
@@ -225,10 +225,10 @@ void bad_deletes()
delete [0] (int*)0; // expected-error {{expected variable name or 'this' in lambda capture list}}
delete (void*)0;
// cxx98-23-warning@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete (T*)0;
- // cxx98-23-warning@-1 {{deleting pointer to incomplete type}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
+ // cxx98-23-warning@-1 {{deleting pointer to incomplete struct 'T'}}
+ // since-cxx26-error@-2 {{cannot delete pointer to incomplete struct 'T'}}
::S::delete (int*)0; // expected-error {{expected unqualified-id}}
}
@@ -570,8 +570,8 @@ namespace DeleteIncompleteClassPointerError {
struct A; // expected-note {{forward declaration}}
void f(A *x) { 1+delete x; }
// expected-error@-1 {{invalid operands to binary expression}}
- // cxx98-23-warning@-2 {{deleting pointer to incomplete type}}
- // since-cxx26-error@-3 {{cannot delete pointer to incomplete type 'A'}}
+ // cxx98-23-warning@-2 {{deleting pointer to incomplete struct 'A'}}
+ // since-cxx26-error@-3 {{cannot delete pointer to incomplete struct 'A'}}
}
namespace PR10504 {
@@ -595,6 +595,10 @@ struct GH99278_2 {
} f;
};
GH99278_2<void> e;
+void GH99278_3(int(*p)[]) {
+ delete p;
+ // expected-warning@-1 {{'delete' applied to a pointer-to-array type 'int (*)[]' treated as 'delete[]'}}
+};
#endif
struct PlacementArg {};
|
|
@llvm/pr-subscribers-clang Author: Harald van Dijk (hvdijk) ChangesP3144R2 made it ill-formed to delete a pointer to an incomplete class type, and asserted that incomplete class types were the only incomplete types that were possible to pass to the delete operator. This assertion was wrong, and PR 99278 already updated the check to exclude incomplete enum types, but that is still wrong: pointers to arrays of unknown length are another instance of incomplete types that are possible to delete and were not included in PR3144R2's ban. Additionally, the diagnostic still claimed that the problem was with deleting pointers to incomplete types, rather than to incomplete class types. This PR ensures that when we implement the check for incomplete class type, we only check for incomplete class type, and adjusts the diagnostics to say 'incomplete struct' or 'incomplete union' to be accurate without being overly verbose. Full diff: https://github.com/llvm/llvm-project/pull/149406.diff 7 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b2ea65ae111be..d86f10da17c61 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8392,17 +8392,19 @@ def ext_default_init_const : ExtWarn<
"is a Microsoft extension">,
InGroup<MicrosoftConstInit>;
def err_delete_operand : Error<"cannot delete expression of type %0">;
+def err_delete_void_ptr_operand : Error<
+ "cannot delete expression with pointer-to-'void' type %0">;
def ext_delete_void_ptr_operand : ExtWarn<
"cannot delete expression with pointer-to-'void' type %0">,
InGroup<DeleteIncomplete>;
def err_ambiguous_delete_operand : Error<
"ambiguous conversion of delete expression of type %0 to a pointer">;
def warn_delete_incomplete : Warning<
- "deleting pointer to incomplete type %0 is incompatible with C++2c"
+ "deleting pointer to incomplete %select{struct|union}0 %1 is incompatible with C++2c"
" and may cause undefined behavior">,
InGroup<DeleteIncomplete>;
def err_delete_incomplete : Error<
- "cannot delete pointer to incomplete type %0">;
+ "cannot delete pointer to incomplete %select{struct|union}0 %1">;
def err_delete_incomplete_class_type : Error<
"deleting incomplete class type %0; no conversions to pointer type">;
def err_delete_explicit_conversion : Error<
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index f851c9e1d5015..a741acc8a0a89 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -4033,25 +4033,24 @@ Sema::ActOnCXXDelete(SourceLocation StartLoc, bool UseGlobal,
// effectively bans deletion of "void*". However, most compilers support
// this, so we treat it as a warning unless we're in a SFINAE context.
// But we still prohibit this since C++26.
- Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_incomplete
+ Diag(StartLoc, LangOpts.CPlusPlus26 ? diag::err_delete_void_ptr_operand
: diag::ext_delete_void_ptr_operand)
- << (LangOpts.CPlusPlus26 ? Pointee : Type)
- << Ex.get()->getSourceRange();
+ << Type << Ex.get()->getSourceRange();
} else if (Pointee->isFunctionType() || Pointee->isVoidType() ||
Pointee->isSizelessType()) {
return ExprError(Diag(StartLoc, diag::err_delete_operand)
- << Type << Ex.get()->getSourceRange());
+ << Type << Ex.get()->getSourceRange());
} else if (!Pointee->isDependentType()) {
// FIXME: This can result in errors if the definition was imported from a
// module but is hidden.
- if (Pointee->isEnumeralType() ||
- !RequireCompleteType(StartLoc, Pointee,
- LangOpts.CPlusPlus26
- ? diag::err_delete_incomplete
- : diag::warn_delete_incomplete,
- Ex.get())) {
- if (const RecordType *RT = PointeeElem->getAs<RecordType>())
+ if (const RecordType *RT = PointeeElem->getAs<RecordType>()) {
+ if (!RequireCompleteType(StartLoc, Pointee,
+ LangOpts.CPlusPlus26
+ ? diag::err_delete_incomplete
+ : diag::warn_delete_incomplete,
+ RT->isUnionType(), Ex.get())) {
PointeeRD = cast<CXXRecordDecl>(RT->getDecl());
+ }
}
}
diff --git a/clang/test/Analysis/dtor.cpp b/clang/test/Analysis/dtor.cpp
index c17c886d97fb4..66f555cd65240 100644
--- a/clang/test/Analysis/dtor.cpp
+++ b/clang/test/Analysis/dtor.cpp
@@ -499,7 +499,7 @@ namespace PseudoDtor {
namespace Incomplete {
class Foo; // expected-note{{forward declaration}}
- void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete type}}
+ void f(Foo *foo) { delete foo; } // expected-warning{{deleting pointer to incomplete struct 'Foo'}}
}
namespace TypeTraitExpr {
diff --git a/clang/test/CXX/drs/cwg5xx.cpp b/clang/test/CXX/drs/cwg5xx.cpp
index 1d505adecfb27..269397a0dc221 100644
--- a/clang/test/CXX/drs/cwg5xx.cpp
+++ b/clang/test/CXX/drs/cwg5xx.cpp
@@ -907,7 +907,7 @@ namespace cwg573 { // cwg573: no
// cxx98-error@-1 {{cast between pointer-to-function and pointer-to-object is an extension}}
void f() { delete a; }
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
int n = d - a;
// expected-error@-1 {{arithmetic on pointers to void}}
// FIXME: This is ill-formed.
@@ -1298,12 +1298,12 @@ namespace cwg599 { // cwg599: partial
void f(void *p, void (*q)(), S s, T t, U u, V v) {
delete p;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete q;
// expected-error@-1 {{cannot delete expression of type 'void (*)()'}}
delete s;
// cxx98-23-error@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete t;
// expected-error@-1 {{cannot delete expression of type 'T'}}
// FIXME: This is valid, but is rejected due to a non-conforming GNU
diff --git a/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp b/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
index ecb29189af60b..4e68fbab5d670 100644
--- a/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
+++ b/clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
@@ -6,12 +6,12 @@
// The trivial case.
class T0; // expected-note {{forward declaration}}
-void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete type}}
+void f0(T0 *a) { delete a; } // expected-warning {{deleting pointer to incomplete struct 'T0'}}
class T0 { ~T0(); };
// The trivial case, inside a template instantiation.
template<typename T>
-struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete type}}
+struct T1_A { T *x; ~T1_A() { delete x; } }; // expected-warning {{deleting pointer to incomplete struct 'T1_B'}}
class T1_B; // expected-note {{forward declaration}}
void f0() { T1_A<T1_B> x; } // expected-note {{in instantiation of member function}}
@@ -44,3 +44,8 @@ class T3_A {
private:
~T3_A(); // expected-note{{declared private here}}
};
+
+// The trivial case but with a union.
+union T4; // expected-note {{forward declaration}}
+void f4(T4 *a) { delete a; } // expected-warning {{deleting pointer to incomplete union 'T4'}}
+union T4 { ~T4(); };
diff --git a/clang/test/OpenMP/deferred-diags.cpp b/clang/test/OpenMP/deferred-diags.cpp
index e31b99b8c88e4..a978465322e9d 100644
--- a/clang/test/OpenMP/deferred-diags.cpp
+++ b/clang/test/OpenMP/deferred-diags.cpp
@@ -41,7 +41,7 @@ namespace TestDeleteIncompleteClassDefinition {
struct a;
struct b {
b() {
- delete c; // expected-warning {{deleting pointer to incomplete type 'a' is incompatible with C++2c and may cause undefined behavior}}
+ delete c; // expected-warning {{deleting pointer to incomplete struct 'a' is incompatible with C++2c and may cause undefined behavior}}
}
a *c;
};
diff --git a/clang/test/SemaCXX/new-delete.cpp b/clang/test/SemaCXX/new-delete.cpp
index f918501554f80..735908c5dc0bb 100644
--- a/clang/test/SemaCXX/new-delete.cpp
+++ b/clang/test/SemaCXX/new-delete.cpp
@@ -225,10 +225,10 @@ void bad_deletes()
delete [0] (int*)0; // expected-error {{expected variable name or 'this' in lambda capture list}}
delete (void*)0;
// cxx98-23-warning@-1 {{cannot delete expression with pointer-to-'void' type 'void *'}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'void'}}
+ // since-cxx26-error@-2 {{cannot delete expression with pointer-to-'void' type 'void *'}}
delete (T*)0;
- // cxx98-23-warning@-1 {{deleting pointer to incomplete type}}
- // since-cxx26-error@-2 {{cannot delete pointer to incomplete type 'T'}}
+ // cxx98-23-warning@-1 {{deleting pointer to incomplete struct 'T'}}
+ // since-cxx26-error@-2 {{cannot delete pointer to incomplete struct 'T'}}
::S::delete (int*)0; // expected-error {{expected unqualified-id}}
}
@@ -570,8 +570,8 @@ namespace DeleteIncompleteClassPointerError {
struct A; // expected-note {{forward declaration}}
void f(A *x) { 1+delete x; }
// expected-error@-1 {{invalid operands to binary expression}}
- // cxx98-23-warning@-2 {{deleting pointer to incomplete type}}
- // since-cxx26-error@-3 {{cannot delete pointer to incomplete type 'A'}}
+ // cxx98-23-warning@-2 {{deleting pointer to incomplete struct 'A'}}
+ // since-cxx26-error@-3 {{cannot delete pointer to incomplete struct 'A'}}
}
namespace PR10504 {
@@ -595,6 +595,10 @@ struct GH99278_2 {
} f;
};
GH99278_2<void> e;
+void GH99278_3(int(*p)[]) {
+ delete p;
+ // expected-warning@-1 {{'delete' applied to a pointer-to-array type 'int (*)[]' treated as 'delete[]'}}
+};
#endif
struct PlacementArg {};
|
hvdijk
requested review from
cor3ntin and
Endilll
and removed request for
Endilll
|
Apologies for the repeated review request, not sure why GitHub removed one when I only tried to request an additional reviewer. |
| def err_delete_void_ptr_operand : Error< | ||
| "cannot delete expression with pointer-to-'void' type %0">; |
There was a problem hiding this comment.
It would be more consistent to say type 'void*'
There are just a couple of diagnostics mentioning "pointer to void" - but we never say pointer-to-void, let alone "pointer-to-'void' - that way it can be merged with err_delete_incomplete ie {|class}or{|class|union}` (see below)
There was a problem hiding this comment.
This text is literally copied and pasted from what it says directly below for the pre-C++26 case where it is a warning rather than an error, it's not true that we never say pointer-to-void.
| def warn_delete_incomplete : Warning< | ||
| "deleting pointer to incomplete type %0 is incompatible with C++2c" | ||
| "deleting pointer to incomplete %select{struct|union}0 %1 is incompatible with C++2c" |
There was a problem hiding this comment.
I'm not sure there is value in distinguishing unions here; unions are class types.
So either always say class types, or {class|union}0 %1
There was a problem hiding this comment.
A diagnostic referring to a union U as "struct 'U'" or "class 'U'" would be misleading. Although they are class types, the struct or class keyword cannot be used for them. %select{struct|union} is used in other diagnostics to avoid confusion, e.g. "%select{struct|union}0 kernel parameters may not contain pointers".
But I see in yet other places that it's making sure to use either struct or class depending on what the user wrote. That would likely be even clearer for users, so will update.
There was a problem hiding this comment.
In C++, types introduced by struct, class, and union are all "class types". And for this warning, the distinction does not matter, as they are all treated the same way. So I don't think the added complexity is warranted here
There was a problem hiding this comment.
I'm not going to add diagnostics that are technically correct but will confuse users.
There was a problem hiding this comment.
IMO, class type here is confusing in the case of the 'other' kinds. That said, I don't see the value in saying anything other than incomplete type. The fact that it is an enum, class, struct, union/etc type is not a helpful addition, and I'd say to just revert both of the diagnostics changes.
There was a problem hiding this comment.
That said, I don't see the value in saying anything other than incomplete type.
My reason for wanting to change the diagnostic is that the diagnostic is telling users that it is an error to delete a pointer to an incomplete type when that is not the case.
That said, I'd be okay with taking the diagnostic changes out of this PR and leaving this as just the fix for the rejects-valid. We can figure out whether we can come up with improved wording in a separate PR.
There was a problem hiding this comment.
Done, with this update, the diagnostics are exactly as they were before. I've kept the other updates to the tests, but adjusted the message that is being checked for. If this looks okay we can just merge this. I can open a new issue or PR for the diagnostic later, but since that has proved contentious I'll make sure to write in far more detail about the different options and their advantages and disadvantages.
| " and may cause undefined behavior">, | ||
| InGroup<DeleteIncomplete>; | ||
| def err_delete_incomplete : Error< | ||
| "cannot delete pointer to incomplete type %0">; | ||
| "cannot delete pointer to incomplete %select{struct|union}0 %1">; |
There is no separate GitHub issue for this, but will add a release note. |
* Use PointeeRD->getTagKind() instead of RT->isUnionType() for the tag. * Add release note.
|
Release note added, and diagnostics updated to use I do take your point about |
|
How about this? For an incomplete class C, we used to generate the misleading "cannot delete pointer to incomplete type 'C'", now "cannot delete pointer to incomplete class 'C'", for a pointer to void, matching wording can just be "cannot delete pointer to 'void'". Does that look acceptable? It's possible to merge err_delete_incomplete and err_delete_void_ptr_operand as you suggested but I think that requires a bit more complexity in err_delete_incomplete, or accepting misleading diagnostics. |
hvdijk
changed the title
P3144R2 made it ill-formed to delete a pointer to an incomplete class type, and asserted that incomplete class types were the only incomplete types that were possible to pass to the delete operator. This assertion was wrong, and PR 119077 already updated the check to exclude incomplete enum types, but that is still wrong: pointers to arrays of unknown length are another instance of incomplete types that are possible to delete and were not included in P3144R2's ban.
This PR ensures that when we implement the check for incomplete class type, we only check for incomplete class type.