Skip to content

One more fix for P3144R2 implementation #149406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+31 −14
Open

One more fix for P3144R2 implementation #149406

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
94e652b
More fixes for P3144R2 implementation
hvdijk Jul 17, 2025
a2123f9
Update PR.
hvdijk Jul 18, 2025
69a65b2
Also tweak wording of deleting pointers to void.
hvdijk Jul 18, 2025
2f751bc
Revert diagnostics changes.
hvdijk Jul 18, 2025
6c21226
Add delete pointer to array of class test
hvdijk Jul 22, 2025
eff9ccb
Change array deletion tests to use delete [].
hvdijk Jul 23, 2025
268265f
One more.
hvdijk Jul 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not going to add diagnostics that are technically correct but will confuse users.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">;
Copy link
Contributor

@cor3ntin cor3ntin Jul 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

def err_delete_incomplete_class_type : Error<
"deleting incomplete class type %0; no conversions to pointer type">;
def err_delete_explicit_conversion : Error<
Expand Down
21 changes: 10 additions & 11 deletions clang/lib/Sema/SemaExprCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/dtor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 3 additions & 3 deletions clang/test/CXX/drs/cwg5xx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
9 changes: 7 additions & 2 deletions clang/test/CXX/expr/expr.unary/expr.delete/p5.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}

Expand Down Expand Up @@ -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(); };
2 changes: 1 addition & 1 deletion clang/test/OpenMP/deferred-diags.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
14 changes: 9 additions & 5 deletions clang/test/SemaCXX/new-delete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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[]'}}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also test deleting an incomplete array of a complete class type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that made me think, what about a pointer to an array of incomplete class type? If that's done through delete[], it would be ill-formed. If that's done through delete, the compiler tells the user that it's treated as delete[], yet it would not be ill-formed. That does not make sense and I am not sure what's the right thing to do there is.

Comparing to what GCC does, GCC doesn't accept this example (even with int(*)[]).

The example does not appear to violate any rule (at least when the pointer is a null pointer) and clearly was not intended to be banned by P3144R2, yet at the same time I am getting the feeling it never should have been allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What EDG does (thanks Compiler Explorer for having that available) is allow it for arrays of complete class type, and disallow it for arrays of incomplete class type. That seems like a sensible way of going about it, it effectively interprets "If the object being deleted has incomplete class type at the point of deletion, the program is ill-formed" as applying also to subobjects of the object being deleted. Which is not what the standard says, but necessary for what P3144R2 aimed to accomplish, so I'll see if I can implement that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That turned out to be a trivial one-line change, so I've done it and added tests for it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general issue here is that there's a hole in the standard: a non-array new-expression can't return a pointer to an array, so [expr.delete]p2 effectively says it's always undefined behavior to delete a pointer to an array (unless it's null). Existing compilers treat this as if you wrote delete[]. (clang and EDG warn.) This is a problem whether or not the array is incomplete.

Probably we should ask the committee to address this.

That said, given the current state of things, this patch seems fine.

On a sort of related note, the following currently crashes in codegen:

struct A { ~A(); };
void f(A (*a)[]){ delete[] a; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general issue here is that there's a hole in the standard: a non-array new-expression can't return a pointer to an array, so [expr.delete]p2 effectively says it's always undefined behavior to delete a pointer to an array (unless it's null).

True for the tests that I added with delete. For your example with delete[], consider:

struct S { ~S(); };
void del(S(*p)[]) { delete[] p; }
void test() { del(new S[4][4]); }

The checks that I implemented in this PR handle this, they allow it in that test, but reject it if S is incomplete which is necessary to avoid UB.

The codegen crash is worrying, the test I'm showing here shows that this can occur in legitimate code. I'll have a look at that when I have some extra time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the array tests to use delete [] so that they test something useful, so that they test something that could be valid even for non-null pointers.

#endif

struct PlacementArg {};
Expand Down
Loading