Destructors #1154

josh11b · 2022-03-25T19:18:45Z

Add a way for classes to add custom destructor code:

class Class1 {
  // forward declaration
  destructor [me: Self] { ... }
}
// out-of-line definition
destructor Class1 [me: Self] { ... }

class Class2 {
  // can modify `me` as part of destruction
  destructor [addr me: Self*] { ... }
}
base class MyBaseClass {
  virtual destructor [addr me: Self*] { ... }
}
class MyDerivedClass extend MyBaseClass {
  impl destructor [addr me: Self*] { ... }
}

and constraints Concrete, Deletable, Destructible, and TrivialDestructor.

chandlerc

First round of review comments. Generally this is looking like a good step here.

chandlerc · 2022-04-01T22:10:16Z

docs/design/classes.md

+Whether the destructor is defined implicitly or explicitly, the compiler
+implements the
+[`Destructible`](/docs/design/generics/details.md#destructor-constraints)
+[type-of-type](/docs/design/generics/terminology.md#type-of-type). It is illegal
+to directly implement `Destructible` for a type.


Would this be clearer? It is to me, but that may just be me... or worse, it may represent me misunderstanding the proposal.

Suggested change

Whether the destructor is defined implicitly or explicitly, the compiler

implements the

[`Destructible`](/docs/design/generics/details.md#destructor-constraints)

[type-of-type](/docs/design/generics/terminology.md#type-of-type). It is illegal

to directly implement `Destructible` for a type.

Whether the destructor is defined implicitly or explicitly, the compiler

implements the

[`Destructible`](/docs/design/generics/details.md#destructor-constraints)

[type-of-type](/docs/design/generics/terminology.md#type-of-type). This

type-of-type doesn't introduce any new names. It is also not an

[interface](/docs/design/generics/terminology.md#interface) and so cannot

be implemented directly.

Technically you can implement some non-interfaces for types and there are some interfaces you can't implement for types (for example if they are private to some other library). It seemed better to say what was relevant: it can't be implemented directly. Whether the type-of-type is an interface or not should not actually be observable.

I added a sentence about names.

docs/design/classes.md

chandlerc · 2022-04-06T07:50:12Z

docs/design/classes.md

+-   Final classes and classes with a virtual destructor are `Deletable`. These
+    may be safely deleted through a pointer.


I would like to make the nomenclature here tentative, but not block on resolving it.

Specifically, I'd like to revisit the term Delete in the Allocator interface below but in conjunction with the allocation interface and pick a set of names that seem cohesive. Maybe it is Delete, maybe it is something different. But I don't think that name is very important to this design, and I don't think using Delete for now will be confusing at all. I'd just suggest updating all the terms to align with each other at the end rather than stress about it here.

As part of that, I think we'd also want to reflect #1058 here and whether Deletable (or whatever other formulation) is going to be hard for folks to spell correctly. But it seems like a waste of time to stress about that until we know the core terminology in the allocator interface and can build off of that.

Does that work for you (and others)?

I can mark this term as provisional until the allocator interface goes through the proposal process. Deletable does not conform to the recommendation in #1058 though, so it seems like some change should be made now. The top candidates for the interface name seem to be: Delete, CanDelete, DeleteInterface, DeleteConstraint

My suggestion is just to leave this as Deletable and Destructible but mark them as provisional rather than stalling this PR on the naming issue. We can have an issue to track so we don't forget, but I spent some time trying to think of a good option, or to convince myself about ane of the ones you list, and they all seem off -- enough that I'm uncomfortable even churning to one of them tentatively (I like "Deletable" better, but it doesn't match our strategy).

That OK? (Both for you and the other @carbon-language/carbon-leads )

Okay, I've added a note in a couple places and an alternative to this proposal.

Confirmed with at least one other lead, LGTM.

chandlerc · 2022-04-06T07:56:52Z

docs/design/classes.md

+    object of the member variable is destroyed.
+-   Final classes and classes with a virtual destructor are `Deletable`. These
+    may be safely deleted through a pointer.
+-   Classes that are `Instantiable`, `Deletable`, or both are `Destructible`.


I both really like the term Destructible and am simultaneously worried about its spelling. I don't know how easy it will be for folks to remember.

The direction we've been leaning in in #1058 would lean away from this name for this exact reason. But I don't have a lot of great alternative ideas in this specific case. DestructorConstraint is ... a lot. It also doesn't actually say what it means.

Similarly, HasDestructor seems appealing, but isn't actually the right thing AFAICT.

Instead, I think we want CanDestroy. The type-of-type is about the action of destroying an object, not the destructor itself.

But interestingly, it seems like this is only needed as a type-of-type when doing unsafe deletion? Should we instead just call this the same thing as the other type-of-type but add Unsafe... prefix? I somewhat like that because it doesn't seem like there is any other reason to use this type-of-type.

Are you saying UnsafeCanDestroy? Or would CanDestroyUnsafe read a bit more clearly?

I think I understand better that you mean Unsafe followed by whatever name we pick instead of Deletable

Sorry, GitHub hid this thread from me, but my comment is somewhat the same as above -- I'd rather just revisit these names in a follow-up rather than churn more here as the design is I think good and the names are really easy to adapt after the fact.

Okay, I've added a note in a couple places and an alternative to this proposal.

docs/design/classes.md

proposals/p1154.md

zygoloid

This proposal looks to be in great shape to me.

zygoloid · 2022-04-13T19:34:51Z

docs/design/classes.md

+[`Destructible`](/docs/design/generics/details.md#destructor-constraints)
+[type-of-type](/docs/design/generics/terminology.md#type-of-type). This
+type-of-type does not have any named members. It is illegal to directly
+implement `Destructible` for a type.


I think it'd be good to say here whether it's also illegal to directly implement Concrete or Deletable for a type.

(I see that you disallow this in generics/details.md, but the next paragraph seems to be parallel to this one and doesn't mention this rule, which I initially read as implying that Deletable can be directly implemented by a type.)

I rewrote these few paragraphs to remove redundant information and add that.

zygoloid · 2022-04-13T19:36:41Z

docs/design/classes.md

+classes. It is illegal if that method is abstract and not implemented in the
+current class.


"illegal" sounds like a compile-time check, but this doesn't seem like something that can be checked at compile time in general. What's the intent?

I've changed it to say "abort the execution of the program", but I'm open to other options.

zygoloid · 2022-04-13T19:44:15Z

docs/design/classes.md

+-   the class declaration does not define a destructor or the class defines the
+    destructor with an empty body `{ }`,
+-   all data members implement `TrivialDestructor`, and
+-   all base classes implement `TrivialDestructor`.


Something that comes up (rarely) in C++ is a desire to give one of two different bodies to a destructor of a parameterized class based on properties of the parameter. For example, this happens in std::optional<T>, where we want a body if (holds_value) value.~T(); if the destructor of T is not trivial, and we want a trivial destructor otherwise.

In C++17 and before, people faked this up by having optional<T> derive from one of two different base classes depending on T, but that was pretty messy and complicated. In C++20 onwards, a class is allowed to have multiple declared ("tentative") destructors with different constraints:

template<typename T> class optional { union { T value; }; bool holds_value; public: // ... ~optional() requires (std::is_trivially_destructible_v<T>) = default; ~optional() requires (!std::is_trivially_destructible_v<T>) { if (holds_value) value.~T(); } };

I'm not sure whether the above pattern is something we'd want to directly support in Carbon, but I think we should have some approach for this problem. Maybe that could be as simple as allowing a type to explicitly implement TrivialDestructor itself:

class Optional(T:! Concrete) { var value: Storage(T); var holds_value: bool; // It's perfectly safe, I assure you impl [U:! TrivialDestructor] Optional(U) as TrivialDestructor {} destructor [me: Self] { if (me.holds_value) value.Destroy(); } }

I see you already mention this in "Alternatives considered", but not with the above approach. It's probably worth considering both options, though I think I prefer the conditional methods approach.

I added an alternative.

chandlerc

I also think this looks quite good, but I'd like to make sure the other leads are OK with just leaving the names as-is and revisiting those together at a later time. I think the design here is solid, and these names aren't going to cause any confusion or trouble in the short term. It'll be easy to update them if/when we decide on different or better names, so I'd rather not churn more here.

chandlerc · 2022-04-16T08:36:07Z

docs/design/classes.md

+-   Final classes and classes with a virtual destructor are `Deletable`. These
+    may be safely deleted through a pointer.


My suggestion is just to leave this as Deletable and Destructible but mark them as provisional rather than stalling this PR on the naming issue. We can have an issue to track so we don't forget, but I spent some time trying to think of a good option, or to convince myself about ane of the ones you list, and they all seem off -- enough that I'm uncomfortable even churning to one of them tentatively (I like "Deletable" better, but it doesn't match our strategy).

That OK? (Both for you and the other @carbon-language/carbon-leads )

chandlerc

LGTM for real this time!

chandlerc · 2022-04-19T00:01:40Z

docs/design/classes.md

+-   Final classes and classes with a virtual destructor are `Deletable`. These
+    may be safely deleted through a pointer.


Confirmed with at least one other lead, LGTM.

Add a way for classes to add custom destructor code: ``` class Class1 { // forward declaration destructor [me: Self] { ... } } // out-of-line definition destructor Class1 [me: Self] { ... } class Class2 { // can modify `me` as part of destruction destructor [addr me: Self*] { ... } } base class MyBaseClass { virtual destructor [addr me: Self*] { ... } } class MyDerivedClass extend MyBaseClass { impl destructor [addr me: Self*] { ... } } ``` and constraints `Concrete`, `Deletable`, `Destructible`, and `TrivialDestructor`.

josh11b added the proposal A proposal label Mar 25, 2022

josh11b requested a review from a team March 25, 2022 19:18

josh11b added 18 commits March 25, 2022 19:18

Filling out template with PR 1154

acc641e

Destructor type-of-types

d49fcfb

Finish first draft of design

d00edcf

Checkpoint progress.

877be03

Checkpoint progress.

9d6b3eb

Checkpoint progress.

1a1d058

interface alternative

b427009

always unsafe delete alternative

33fef57

More alternatives

a390634

Final destructor alternative

d4f4711

Checkpoint progress.

e9e0a36

Checkpoint progress.

77e9fb5

Checkpoint progress.

802ff33

Checkpoint progress.

763ee3b

More on alternatives

b14f132

Checkpoint progress.

ae469c8

Checkpoint progress.

f78e078

Checkpoint progress.

cc7778a

josh11b marked this pull request as ready for review March 31, 2022 20:53

josh11b requested a review from a team as a code owner March 31, 2022 20:53

github-actions bot added the proposal rfc Proposal with request-for-comment sent out label Mar 31, 2022

josh11b requested a review from chandlerc March 31, 2022 20:54

josh11b mentioned this pull request Apr 1, 2022

Private interfaces in public API files #971

Closed

Instantiable implies Sized

5a6c8cd

chandlerc requested changes Apr 6, 2022

View reviewed changes

Instantiable -> Concrete

073c370

josh11b added 4 commits April 7, 2022 22:56

TriviallyDestructible -> TrivialDestructor

da5c0a4

Destructible doesn't have names

ac848c3

Oops, missed rationale before

1e21098

Merge remote-tracking branch 'upstream/trunk' into destruct

f1f1464

zygoloid reviewed Apr 13, 2022

View reviewed changes

josh11b added 4 commits April 14, 2022 16:35

No constraints may be impl directly

9f995b7

Illegal -> abort

ee8b186

Alternative allowing impl of TrivialDestructor

b346c26

Fix formatting

ec9abf2

chandlerc approved these changes Apr 16, 2022

View reviewed changes

josh11b added 2 commits April 18, 2022 21:34

Names of type-of-types are placeholders

1096609

Add link

ce1b2e3

chandlerc approved these changes Apr 19, 2022

View reviewed changes

josh11b merged commit 07a9b75 into carbon-language:trunk Apr 19, 2022

josh11b deleted the destruct branch April 19, 2022 00:39

github-actions bot added proposal accepted Decision made, proposal accepted and removed proposal rfc Proposal with request-for-comment sent out labels Apr 19, 2022

nigeltao mentioned this pull request Jul 21, 2022

new and delete equivalents #1483

Closed

chandlerc added the documentation An issue or proposed change to our documentation label Jan 13, 2023

zygoloid mentioned this pull request Jan 30, 2023

out-of-line destructor syntax seems inconsistent with other declaration forms #2567

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destructors #1154

Destructors #1154

josh11b commented Mar 25, 2022 •

edited

Loading

chandlerc left a comment

chandlerc Apr 1, 2022

josh11b Apr 7, 2022

josh11b Apr 7, 2022

chandlerc Apr 6, 2022

josh11b Apr 14, 2022

chandlerc Apr 16, 2022

josh11b Apr 18, 2022

chandlerc Apr 19, 2022

chandlerc Apr 6, 2022

josh11b Apr 11, 2022

josh11b Apr 14, 2022

chandlerc Apr 16, 2022

josh11b Apr 18, 2022

zygoloid left a comment

zygoloid Apr 13, 2022

josh11b Apr 14, 2022

zygoloid Apr 13, 2022

josh11b Apr 14, 2022

zygoloid Apr 13, 2022

zygoloid Apr 13, 2022

josh11b Apr 14, 2022

chandlerc left a comment

chandlerc Apr 16, 2022

chandlerc left a comment

chandlerc Apr 19, 2022

		- Final classes and classes with a virtual destructor are `Deletable`. These
		may be safely deleted through a pointer.

		classes. It is illegal if that method is abstract and not implemented in the
		current class.

Destructors #1154

Destructors #1154

Conversation

josh11b commented Mar 25, 2022 • edited Loading

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zygoloid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chandlerc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh11b commented Mar 25, 2022 •

edited

Loading