-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Support for calling C++ destructors. #6453
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.
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
base: trunk
Are you sure you want to change the base?
Conversation
danakj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| // CHECK:STDERR: var a: Cpp.AmbiguousDestructor = {}; | ||
| // CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~ | ||
| // CHECK:STDERR: | ||
| // CHECK:STDERR: fail_destroy_nondestroyable.carbon:[[@LINE-8]]:3: error: attempt to use a deleted function [CppInteropParseError] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sorting here is confusing. Maybe just split up the deleted vs ambiguous test cases into separate splits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reordering the two tests was enough to make this more readable; I've done that.
| var a: Cpp.TrivialDestructor = {}; | ||
| //@dump-sem-ir-end | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want a test where we inherit from ProtectedDestructor in Carbon and destroy it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. That works, but deriving from PrivateDestructor also works, so I've added a todo_fail test for that.
| return LookupCopyImpl(context, loc_id, self_type_id, specific_interface); | ||
| } | ||
|
|
||
| if (context.identifiers().Get(interface.name_id.AsIdentifierId()) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note, I'd been thinking that all types would use the TypeCanDestroy logic in order to go through type.destroy, so this is kind of creating a case where TypeCanDestroy would be false but it's actually language-destructible. Not sure what the impact will be, particularly since I'd been expecting Destroy should have a single final impl -- will that work with this kind of approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and for context also note the decided interface from #6124:
interface Destroy {
final fn Op[ref self: Self]() = <some builtin>;
}
|
This PR is temporarily on hold while we decide how we want to model |
Synthesize an impl of
Destroyfor C++ classes in response to impl lookup.