www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - D's Destructors are What Scott Meyers Warned Us About

reply sarn <sarn theartofmachinery.com> writes:
(I'm referring to Scott's 2014 DConf talk: 
https://www.youtube.com/watch?v=KAWA1DuvCnQ)

I was actually preparing a DIP about this when Manu posted to the 
forums about his own related problems with C++ interop.

I traced a bug in some of my D code to my own misunderstanding of 
how D's destructors actually work.  So I did some research and 
discovered a bunch of edge cases with using __dtor, __xdtor and 
hasElaborateDestructor.  I tried reviewing the packages on 
code.dlang.org and some elsewhere (thankfully only about 1% of D 
packages use these functions directly) and it turns out I'm not 
the only one confused.  I don't have time to file bug reports for 
every package, so, if you're responsible for code that handles 
destructors manually, please do a review.  There's a *lot* of 
buggy code out there.

I'm starting this thread to talk about ways to make things 
better, but first the bad news.  Let's use this idiomatic code as 
an example of typical bugs:

void foo(T)(auto ref T t)
{
     ...
     static if (hasElaborateDestructor!T)
     {
         t.__dtor();
     }
     ...
}


__dtor calls the destructor defined by T, but not the destructor 
defined by any members of T (if T is a struct or class).  I know, 
some of you might be thinking, "Silly sarn, that's what __xdtor 
is for, of course!"  Turns out this isn't that widely known or 
understood (__dtor is used in examples in the spec --- 
https://dlang.org/spec/struct.html#assign-overload).  A lot of 
code is only __dtor-aware, and there's at least some code out 
there that checks for both __dtor and __xdtor and mistakenly 
prefers __dtor.  __xdtor only solves the specific problem of also 
destroying members.


The destructor will never be run for classes because 
hasElaborateDestructor is only ever true for structs.  This is 
actually per the documentation, but it's also not well known "on 
the ground" (e.g., a lot of code has meaningless is(T == class) 
or is(T == struct) around hasElaborateDestructor).  The code 
example is obviously a leak if t was emplace()d in non-GC memory, 
but even for GCed classes, it's important for containers to be 
explicit about whether or not they own reference types.


Even if __dtor is run on a class instance, it generally won't run 
the correct destructor because it's not virtual.  (I.e., if T is 
a base class, no destructors for derived classes will be called.) 
  The spec says that D destructors are virtual, but this is 
emulated using runtime type information rather than by using the 
normal virtual function implementation.  __xdtor is the same.


Even if the right derived class __dtor is run, it won't run the 
destructors for any base classes.  The spec says that destructors 
automatically recurse to base classes, but, again, this is 
handled manually by walking RTTI, not by making the destructor 
function itself recurse.


The idiom of checking if something needs destruction before 
destroying it is often implemented incorrectly.  As before, 
hasElaborateDestructor works for structs, but doesn't always work 
as expected for classes.  hasMember!(T, "__dtor") seems to work 
for classes, but doesn't work for a struct that doesn't define a 
destructor, but requires destruction for its members (a case 
that's easy to miss in testing).

It looks like most D programmers think that D destructors work 
like they typically do in C++, just like I did.

Here are some recommendations:
* Really try to just use destroy().  Manually working with 
__dtor/__xdtor is a minefield, and I haven't seen any code that 
actually reimplements the RTTI walk that the runtime library 
does.  (Unfortunately destroy() currently isn't zero-overhead for 
plain old data structs because it's based on RTTI, but at least 
it works.)
* Avoid trying to figure out if something needs destruction.  
Just destroy everything, or make it clear you don't own classes 
at all, or be totally sure you're working with plain old data 
structs.
* Some code uses __dtor as a way to manually run cleanup code on 
an object that will be used again.  Putting this cleanup code 
into a normal method will cause fewer headaches.

The one other usage of these low-level destructor facilities is 
checking if a type is a plain old data struct.  This is an 
important special case for some code, but everyone seems to do 
the check a different way.  Maybe a special isPod trait is needed.
May 22 2018
next sibling parent reply rikki cattermole <rikki cattermole.co.nz> writes:
On 23/05/2018 1:59 PM, sarn wrote:
 (I'm referring to Scott's 2014 DConf talk: 
 https://www.youtube.com/watch?v=KAWA1DuvCnQ)
 
 I was actually preparing a DIP about this when Manu posted to the forums 
 about his own related problems with C++ interop.
 
 I traced a bug in some of my D code to my own misunderstanding of how 
 D's destructors actually work.  So I did some research and discovered a 
 bunch of edge cases with using __dtor, __xdtor and 
 hasElaborateDestructor.  I tried reviewing the packages on 
 code.dlang.org and some elsewhere (thankfully only about 1% of D 
 packages use these functions directly) and it turns out I'm not the only 
 one confused.  I don't have time to file bug reports for every package, 
 so, if you're responsible for code that handles destructors manually, 
 please do a review.  There's a *lot* of buggy code out there.
 
 I'm starting this thread to talk about ways to make things better, but 
 first the bad news.  Let's use this idiomatic code as an example of 
 typical bugs:
 
 void foo(T)(auto ref T t)
 {
      ...
      static if (hasElaborateDestructor!T)
      {
          t.__dtor();
      }
      ...
 }
 

 __dtor calls the destructor defined by T, but not the destructor defined 
 by any members of T (if T is a struct or class).  I know, some of you 
 might be thinking, "Silly sarn, that's what __xdtor is for, of course!"  
 Turns out this isn't that widely known or understood (__dtor is used in 
 examples in the spec --- 
 https://dlang.org/spec/struct.html#assign-overload).  A lot of code is 
 only __dtor-aware, and there's at least some code out there that checks 
 for both __dtor and __xdtor and mistakenly prefers __dtor.  __xdtor only 
 solves the specific problem of also destroying members.
 

 The destructor will never be run for classes because 
 hasElaborateDestructor is only ever true for structs.  This is actually 
 per the documentation, but it's also not well known "on the ground" 
 (e.g., a lot of code has meaningless is(T == class) or is(T == struct) 
 around hasElaborateDestructor).  The code example is obviously a leak if 
 t was emplace()d in non-GC memory, but even for GCed classes, it's 
 important for containers to be explicit about whether or not they own 
 reference types.
 

 Even if __dtor is run on a class instance, it generally won't run the 
 correct destructor because it's not virtual.  (I.e., if T is a base 
 class, no destructors for derived classes will be called.)  The spec 
 says that D destructors are virtual, but this is emulated using runtime 
 type information rather than by using the normal virtual function 
 implementation.  __xdtor is the same.
 

 Even if the right derived class __dtor is run, it won't run the 
 destructors for any base classes.  The spec says that destructors 
 automatically recurse to base classes, but, again, this is handled 
 manually by walking RTTI, not by making the destructor function itself 
 recurse.
 

 The idiom of checking if something needs destruction before destroying 
 it is often implemented incorrectly.  As before, hasElaborateDestructor 
 works for structs, but doesn't always work as expected for classes.  
 hasMember!(T, "__dtor") seems to work for classes, but doesn't work for 
 a struct that doesn't define a destructor, but requires destruction for 
 its members (a case that's easy to miss in testing).
 
 It looks like most D programmers think that D destructors work like they 
 typically do in C++, just like I did.
 
 Here are some recommendations:
 * Really try to just use destroy().  Manually working with 
 __dtor/__xdtor is a minefield, and I haven't seen any code that actually 
 reimplements the RTTI walk that the runtime library does.  
 (Unfortunately destroy() currently isn't zero-overhead for plain old 
 data structs because it's based on RTTI, but at least it works.)
 * Avoid trying to figure out if something needs destruction. Just 
 destroy everything, or make it clear you don't own classes at all, or be 
 totally sure you're working with plain old data structs.
 * Some code uses __dtor as a way to manually run cleanup code on an 
 object that will be used again.  Putting this cleanup code into a normal 
 method will cause fewer headaches.
 
 The one other usage of these low-level destructor facilities is checking 
 if a type is a plain old data struct.  This is an important special case 
 for some code, but everyone seems to do the check a different way.  
 Maybe a special isPod trait is needed.
I would consider the current state with classes a bug. So ticket please, it should not require a DIP to change (although Walter may disagree).
May 22 2018
parent reply sarn <sarn theartofmachinery.com> writes:
On Wednesday, 23 May 2018 at 02:13:13 UTC, rikki cattermole wrote:
 I would consider the current state with classes a bug.
 So ticket please, it should not require a DIP to change 
 (although Walter may disagree).
Unfortunately, the way __dtor and __xdtor work for classes can't be changed without the risk of breaking code. (Even if the current behaviour is broken, users might be working around it.)
May 23 2018
next sibling parent reply Manu <turkeyman gmail.com> writes:
On 23 May 2018 at 15:47, sarn via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Wednesday, 23 May 2018 at 02:13:13 UTC, rikki cattermole wrote:
 I would consider the current state with classes a bug.
 So ticket please, it should not require a DIP to change (although Walter
 may disagree).
Unfortunately, the way __dtor and __xdtor work for classes can't be changed without the risk of breaking code. (Even if the current behaviour is broken, users might be working around it.)
In what way is the current behaviour *broken*? (as opposed to awkward, confusing, and poorly documented)
May 23 2018
parent reply sarn <sarn theartofmachinery.com> writes:
On Thursday, 24 May 2018 at 12:26:00 UTC, Steven Schveighoffer 
wrote:
 I added the bug report here: 
 https://issues.dlang.org/show_bug.cgi?id=18903. I outline 
 exactly how to fix it, if anyone wants an easy PR opportunity.

 -Steve
Thanks for digging into that. On Wednesday, 23 May 2018 at 23:18:46 UTC, Manu wrote:
 On 23 May 2018 at 15:47, sarn via Digitalmars-d 
 <digitalmars-d puremagic.com> wrote:
 On Wednesday, 23 May 2018 at 02:13:13 UTC, rikki cattermole 
 wrote:
 I would consider the current state with classes a bug.
 So ticket please, it should not require a DIP to change 
 (although Walter
 may disagree).
Unfortunately, the way __dtor and __xdtor work for classes can't be changed without the risk of breaking code. (Even if the current behaviour is broken, users might be working around it.)
In what way is the current behaviour *broken*? (as opposed to awkward, confusing, and poorly documented)
If c is a class instance, then c.__xdtor compiles and runs but can only be guaranteed to run the right destructors in special controlled cases where there's most likely a better way. That std.signals code that Steven filed a bug report for is example of why we can't just fix the behaviour, though. If we just fixed __dtor/__xdtor, any code that used std.signals would start having ugly bugs at run time. I think that longer term we'll have to deprecate and remove these functions.
May 25 2018
parent reply Mike Franklin <slavo5150 yahoo.com> writes:
On Friday, 25 May 2018 at 23:47:33 UTC, sarn wrote:

 That std.signals code that Steven filed a bug report for is 
 example of why we can't just fix the behaviour, though.  If we 
 just fixed __dtor/__xdtor, any code that used std.signals would 
 start having ugly bugs at run time.

 I think that longer term we'll have to deprecate and remove 
 these functions.
I'm very much interested in doing something about these functions. __xdtor is just one. There are others at https://github.com/dlang/druntime/blob/54ab96e9977e0c6baa7ed9740810058fd4aec6ef/src/o ject.d#L1212-L1229. __xtoHash is currently causing problems at https://github.com/dlang/dmd/pull/8222 TypeInfo has become my nemesis. I've been trying to replace runtime hooks that depend on TypeInfo with templates that can get their information at compile-time, but I'm running into all sorts of problems. e.g. Did you know array.length can be set in safe nothrow pure code, but it lowers to runtime functions that are neither safe, nothrow, nor pure? Anyway, I'm getting better at modifying the compiler/runtime interface. If we can come up with a solution to this mess, and I can understand it, I might be able to implement it. Mike
May 27 2018
next sibling parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Sunday, 27 May 2018 at 09:55:56 UTC, Mike Franklin wrote:
 On Friday, 25 May 2018 at 23:47:33 UTC, sarn wrote:

 [...]
I'm very much interested in doing something about these functions. __xdtor is just one. There are others at https://github.com/dlang/druntime/blob/54ab96e9977e0c6baa7ed9740810058fd4aec6ef/src/o ject.d#L1212-L1229. __xtoHash is currently causing problems at https://github.com/dlang/dmd/pull/8222 TypeInfo has become my nemesis. I've been trying to replace runtime hooks that depend on TypeInfo with templates that can get their information at compile-time, but I'm running into all sorts of problems. e.g. Did you know array.length can be set in safe nothrow pure code, but it lowers to runtime functions that are neither safe, nothrow, nor pure? Anyway, I'm getting better at modifying the compiler/runtime interface. If we can come up with a solution to this mess, and I can understand it, I might be able to implement it. Mike
You are replacing runtime typeinfo with compiletime templates. Unless you can guarantee that the type information won't change during runtime, you are going to have a hard time. Alex
May 27 2018
parent reply Mike Franklin <slavo5150 yahoo.com> writes:
On Sunday, 27 May 2018 at 16:06:21 UTC, 12345swordy wrote:

 You are replacing runtime typeinfo with compiletime templates. 
 Unless you can guarantee that the type information won't change 
 during runtime, you are going to have a hard time.
Read this thread for context https://forum.dlang.org/post/mr7a65$2hc$1 digitalmars.com See this PR for an example https://github.com/dlang/dmd/pull/7225 And see this talk for a demonstration of the benefits https://www.youtube.com/watch?v=endKC3fDxqs Mike
May 27 2018
parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Sunday, 27 May 2018 at 18:55:41 UTC, Mike Franklin wrote:
 On Sunday, 27 May 2018 at 16:06:21 UTC, 12345swordy wrote:

 You are replacing runtime typeinfo with compiletime templates. 
 Unless you can guarantee that the type information won't 
 change during runtime, you are going to have a hard time.
Read this thread for context https://forum.dlang.org/post/mr7a65$2hc$1 digitalmars.com See this PR for an example https://github.com/dlang/dmd/pull/7225 And see this talk for a demonstration of the benefits https://www.youtube.com/watch?v=endKC3fDxqs Mike
Can you actually reply to me instead of saying "read/watch this"? I'm not going to watch an hour, just to see how you going to achieve this.
May 27 2018
parent sarn <sarn theartofmachinery.com> writes:
On Sunday, 27 May 2018 at 21:11:42 UTC, 12345swordy wrote:
 On Sunday, 27 May 2018 at 18:55:41 UTC, Mike Franklin wrote:
 And see this talk for a demonstration of the benefits 
 https://www.youtube.com/watch?v=endKC3fDxqs

 Mike
Can you actually reply to me instead of saying "read/watch this"? I'm not going to watch an hour, just to see how you going to achieve this.
tl;dw: there's precedent. Historically typeinfo has been overused in the runtime library (I think it's because a lot of the code/architecture predates today's templates). For example, if you compared an int[] to an int[], the compiler would generate a function that looked up a comparator for int using RTTI, and would iterate over the arrays calling it on each element. That video is about replacing code like that with smarter templated code that's not only faster but results in *less* code bloat.
May 27 2018
prev sibling parent reply sarn <sarn theartofmachinery.com> writes:
On Sunday, 27 May 2018 at 09:55:56 UTC, Mike Franklin wrote:
 TypeInfo has become my nemesis.  I've been trying to replace 
 runtime hooks that depend on TypeInfo with templates that can 
 get their information at compile-time, but I'm running into all 
 sorts of problems.  e.g. Did you know array.length can be set 
 in  safe nothrow pure code, but it lowers to runtime functions 
 that are neither  safe, nothrow, nor pure?
Ouch.
 Anyway, I'm getting better at modifying the compiler/runtime 
 interface.  If we can come up with a solution to this mess, and 
 I can understand it, I might be able to implement it.
I've been meaning to learn more about how the compiler/runtime interface works myself but still haven't got around it. I'm probably going to learn a lot by looking at your PRs. I've been thinking this through a bit, and here's what I've got so far: At first I obviously wanted an all-round "just works" low-level interface for destroying objects. But after looking at how people are using __dtor/__xdtor in real code, and looking at the PRs for destroy(), it's obvious that there's a lot of disagreement about what that means. Let's leave that topic for now. If we just focus on fixing classes, we can add, say, __vdtor (I really don't care much about the name, BTW). This needs to be a normal virtual function in the vtable (preferably in a way that's compatible with C++ ABIs) that runs the user-defined destructor code, then recursively calls destructors for members and base classes. The generation of __vdtor also needs a special case to make attributes work. Something like "an empty destructor has attributes inferred like templated functions are, restricted by any of its bases". For example, this needs to work: extern(C++) { class Base { // Virtual ~this() nogc { } } class Derived { // Not marked pure but is still nogc // (NB: explicitly marking nogc isn't needed in current language) override ~this() nogc { } } class Derived2 : Derived { override ~this() nogc pure { // Marked pure, but still recurses to empty destructors in Derived and Base } } } Right now __vdtor would need to be implemented with a thunk that wraps around __xdtor (like I implemented here: https://theartofmachinery.com/2018/05/27/cpp_classes_in_betterc.html). But if __vdtor is implemented, the D runtime code can be simplified to use __vdtor for classes, then hopefully we can deprecate and remove __dtor and __xdtor for classes, and then __vdtor can become the native destructor implementation.
May 27 2018
parent reply sarn <sarn theartofmachinery.com> writes:
On Sunday, 27 May 2018 at 22:27:52 UTC, sarn wrote:
 I've been thinking this through a bit, and here's what I've got 
 so far:
Here's a tweak that should be implementable without any language changes: Instead of trying to detect an empty destructor, we use a UDA on the class --- call it BaseObject or something. A BaseObject-marked class is meant to be something like Andre's ProtoObject, or a custom alternative base. It must not define a destructor or include members with destructors (could relax this in future but works for now), and must only inherit from other BaseObject-marked classes. With that, __vdtor could be implemented using a template mixin. For a BaseObject class, that would generate an empty virtual __vdtor. For other classes, it would call __xdtor and then (non-virtually) call __vdtor for the superclass as long as it's not a BaseObject class. Can anyone see something I've missed? I think it works with the current type system, makes Andre's ProtoObject possible while supporting subclassing with nogc or whatever, and gives us safe class destructors that could be compatible with C++.
May 27 2018
parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Monday, 28 May 2018 at 04:26:02 UTC, sarn wrote:
 On Sunday, 27 May 2018 at 22:27:52 UTC, sarn wrote:
 I've been thinking this through a bit, and here's what I've 
 got so far:
Here's a tweak that should be implementable without any language changes: Instead of trying to detect an empty destructor, we use a UDA on the class --- call it BaseObject or something. A BaseObject-marked class is meant to be something like Andre's ProtoObject, or a custom alternative base. It must not define a destructor or include members with destructors (could relax this in future but works for now), and must only inherit from other BaseObject-marked classes. With that, __vdtor could be implemented using a template mixin. For a BaseObject class, that would generate an empty virtual __vdtor. For other classes, it would call __xdtor and then (non-virtually) call __vdtor for the superclass as long as it's not a BaseObject class. Can anyone see something I've missed? I think it works with the current type system, makes Andre's ProtoObject possible while supporting subclassing with nogc or whatever, and gives us safe class destructors that could be compatible with C++.
How is __vdtor is going to be called, via destroy or via directly? The issue that I see that your going to create a "BaseObject" for every attribute or combination of said attributes. Which creates way too much code bloat. Even more so with the possibility of adding more attributes in the future.
May 28 2018
parent reply sarn <sarn theartofmachinery.com> writes:
On Monday, 28 May 2018 at 20:13:47 UTC, 12345swordy wrote:
 How is __vdtor is going to be called, via destroy or via 
 directly?
Code using destroy() can still use destroy(). Otherwise, __vdtor would be callable in the same way that __dtor and __xdtor are. The plan is to have a better, safer, less problematic (with attributes) solution to what __dtor and __xdtor do with classes, so that longer term we can deprecate and remove the old way.
 The issue that I see that your going to create a "BaseObject" 
 for every attribute or combination of said attributes. Which 
 creates way too much code bloat. Even more so with the 
 possibility of adding more attributes in the future.
So, I won't say the problem doesn't exist, but it's nowhere near as bad as that. The idea isn't to have a BaseObject for every possible combination of attributes. The BaseObject UDA is just a solution to the problem destructors have with inheritance+attributes. Once you have a base class marked BaseObject, you can derive from it and add whatever combination of attributes you like. The BaseObject-marked class can have as much functionality as needed, except it can't have a non-trivial destructor. That's okay for the immediate problems of implementing ProtoObject and __vdtor, and making safe containers, and improving C++ interop and -betterC code. If needed in future, destructor support could be added in the existing language by exploiting attribute inference, but maybe by that time it would be better to make language changes and turn BaseObject into a no-op. It doesn't matter. I just think it's better to have a viable migration plan that doesn't start with "let's make all these language changes first".
May 28 2018
parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Monday, 28 May 2018 at 22:37:01 UTC, sarn wrote:
 On Monday, 28 May 2018 at 20:13:47 UTC, 12345swordy wrote:
 [...]
Code using destroy() can still use destroy(). Otherwise, __vdtor would be callable in the same way that __dtor and __xdtor are. [...]
Interesting... You don't mind me asking your assistance on writing DIP on this? I have one set up already, and I needed help as 1.) This is my first time writing a DIP 2.) I don't know what main course of action to take regarding this issue.
May 28 2018
parent sarn <sarn theartofmachinery.com> writes:
On Monday, 28 May 2018 at 22:53:03 UTC, 12345swordy wrote:
 Interesting... You don't mind me asking your assistance on 
 writing DIP on this?  I have one set up already, and I needed 
 help as
 1.) This is my first time writing a DIP
 2.) I don't know what main course of action to take regarding 
 this issue.
I want to write a draft of a DIP related to attribute inference next chance I get (possibly this weekend). It's related to this, so I'll post the draft here. You're very welcome to comment (or send PRs). BTW, I'll just be following these guidelines and using accepted DIPs as examples: https://github.com/dlang/DIPs/blob/master/GUIDELINES.md https://github.com/dlang/DIPs/tree/master/DIPs/accepted
May 30 2018
prev sibling parent reply Rubn <where is.this> writes:
On Wednesday, 23 May 2018 at 22:47:21 UTC, sarn wrote:
 On Wednesday, 23 May 2018 at 02:13:13 UTC, rikki cattermole 
 wrote:
 I would consider the current state with classes a bug.
 So ticket please, it should not require a DIP to change 
 (although Walter may disagree).
Unfortunately, the way __dtor and __xdtor work for classes can't be changed without the risk of breaking code. (Even if the current behaviour is broken, users might be working around it.)
With the recent poll that was taken, D users seem to be alright with fixing problems if it means breaking code. Especially if it is to fix something that is broken to begin with. Not fixing something that's broken because people might have workarounds implemented for it seems kind of backwards to me.
May 23 2018
parent Manu <turkeyman gmail.com> writes:
On 23 May 2018 at 16:34, Rubn via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Wednesday, 23 May 2018 at 22:47:21 UTC, sarn wrote:
 On Wednesday, 23 May 2018 at 02:13:13 UTC, rikki cattermole wrote:
 I would consider the current state with classes a bug.
 So ticket please, it should not require a DIP to change (although Walter
 may disagree).
Unfortunately, the way __dtor and __xdtor work for classes can't be changed without the risk of breaking code. (Even if the current behaviour is broken, users might be working around it.)
With the recent poll that was taken, D users seem to be alright with fixing problems if it means breaking code. Especially if it is to fix something that is broken to begin with. Not fixing something that's broken because people might have workarounds implemented for it seems kind of backwards to me.
I'm actively working on this stuff at the moment... if you guys can articulate what is *broken*, then I will look at it.
May 23 2018
prev sibling next sibling parent reply Manu <turkeyman gmail.com> writes:
On 22 May 2018 at 18:59, sarn via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 (I'm referring to Scott's 2014 DConf talk:
 https://www.youtube.com/watch?v=KAWA1DuvCnQ)

 I was actually preparing a DIP about this when Manu posted to the forums
 about his own related problems with C++ interop.

 I traced a bug in some of my D code to my own misunderstanding of how D's
 destructors actually work.  So I did some research and discovered a bunch of
 edge cases with using __dtor, __xdtor and hasElaborateDestructor.  I tried
 reviewing the packages on code.dlang.org and some elsewhere (thankfully only
 about 1% of D packages use these functions directly) and it turns out I'm
 not the only one confused.  I don't have time to file bug reports for every
 package, so, if you're responsible for code that handles destructors
 manually, please do a review.  There's a *lot* of buggy code out there.

 I'm starting this thread to talk about ways to make things better, but first
 the bad news.  Let's use this idiomatic code as an example of typical bugs:

 void foo(T)(auto ref T t)
 {
     ...
     static if (hasElaborateDestructor!T)
     {
         t.__dtor();
     }
     ...
 }


 __dtor calls the destructor defined by T, but not the destructor defined by
 any members of T (if T is a struct or class).  I know, some of you might be
 thinking, "Silly sarn, that's what __xdtor is for, of course!"  Turns out
 this isn't that widely known or understood (__dtor is used in examples in
 the spec --- https://dlang.org/spec/struct.html#assign-overload).  A lot of
 code is only __dtor-aware, and there's at least some code out there that
 checks for both __dtor and __xdtor and mistakenly prefers __dtor.  __xdtor
 only solves the specific problem of also destroying members.


 The destructor will never be run for classes because hasElaborateDestructor
 is only ever true for structs.  This is actually per the documentation, but
 it's also not well known "on the ground" (e.g., a lot of code has
 meaningless is(T == class) or is(T == struct) around
 hasElaborateDestructor).  The code example is obviously a leak if t was
 emplace()d in non-GC memory, but even for GCed classes, it's important for
 containers to be explicit about whether or not they own reference types.


 Even if __dtor is run on a class instance, it generally won't run the
 correct destructor because it's not virtual.  (I.e., if T is a base class,
 no destructors for derived classes will be called.)  The spec says that D
 destructors are virtual, but this is emulated using runtime type information
 rather than by using the normal virtual function implementation.  __xdtor is
 the same.


 Even if the right derived class __dtor is run, it won't run the destructors
 for any base classes.  The spec says that destructors automatically recurse
 to base classes, but, again, this is handled manually by walking RTTI, not
 by making the destructor function itself recurse.


 The idiom of checking if something needs destruction before destroying it is
 often implemented incorrectly.  As before, hasElaborateDestructor works for
 structs, but doesn't always work as expected for classes.  hasMember!(T,
 "__dtor") seems to work for classes, but doesn't work for a struct that
 doesn't define a destructor, but requires destruction for its members (a
 case that's easy to miss in testing).

 It looks like most D programmers think that D destructors work like they
 typically do in C++, just like I did.

 Here are some recommendations:
 * Really try to just use destroy().  Manually working with __dtor/__xdtor is
 a minefield, and I haven't seen any code that actually reimplements the RTTI
 walk that the runtime library does.  (Unfortunately destroy() currently
 isn't zero-overhead for plain old data structs because it's based on RTTI,
 but at least it works.)
 * Avoid trying to figure out if something needs destruction.  Just destroy
 everything, or make it clear you don't own classes at all, or be totally
 sure you're working with plain old data structs.
 * Some code uses __dtor as a way to manually run cleanup code on an object
 that will be used again.  Putting this cleanup code into a normal method
 will cause fewer headaches.

 The one other usage of these low-level destructor facilities is checking if
 a type is a plain old data struct.  This is an important special case for
 some code, but everyone seems to do the check a different way.  Maybe a
 special isPod trait is needed.
All of these things! I learned a lot of them over the last few days while trying to implement destructor linkage and virtual destructors for extern(C++). If you're in the mood to prepare a DIP, I think you should prepare this dip: Support the syntax: classInstance.~this(); Which **does the right thing**. That is, aggregate of all the gotchas above! It should recurse the ClassInfo calling the dtors there to perform a full virtual destruction. It should also work for structs. It should also work for extern(C++), which is going to behave yet differently to the cases you've described (since it needs to match C++ semantics)
May 22 2018
next sibling parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
 It should recurse the ClassInfo calling the dtors there to 
 perform a
 full virtual destruction.
Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time) Alexander
May 22 2018
parent reply Manu <turkeyman gmail.com> writes:
On 22 May 2018 at 19:39, 12345swordy via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
 It should recurse the ClassInfo calling the dtors there to perform a
 full virtual destruction.
Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time)
... what?
May 22 2018
parent 12345swordy <alexanderheistermann gmail.com> writes:
On Wednesday, 23 May 2018 at 03:45:39 UTC, Manu wrote:
 On 22 May 2018 at 19:39, 12345swordy via Digitalmars-d 
 <digitalmars-d puremagic.com> wrote:
 On Wednesday, 23 May 2018 at 02:15:25 UTC, Manu wrote:
 It should recurse the ClassInfo calling the dtors there to 
 perform a full virtual destruction.
Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which your suggestion to be attribute unfriendly.(Unless the attributes are determine at compile time)
... what?
Why? That seems restrictive as there is possibility that the parent class have an empty destruction with no attributes which causes your classInstance.~this() suggestion to be attribute unfriendly.(Unless the parent class attributes are determine at compile time) *There I fixed it! Really wish this is a proper web forum.
May 23 2018
prev sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/22/18 10:15 PM, Manu wrote:
 If you're in the mood to prepare a DIP, I think you should prepare this dip:
 
 Support the syntax:
    classInstance.~this();
Isn't this just classInstance.destroy() ? -Steve
May 23 2018
parent reply Manu <turkeyman gmail.com> writes:
On 23 May 2018 at 05:38, Steven Schveighoffer via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On 5/22/18 10:15 PM, Manu wrote:
 If you're in the mood to prepare a DIP, I think you should prepare this
 dip:

 Support the syntax:
    classInstance.~this();
Isn't this just classInstance.destroy() ?
Is everything in object.d globally available? You don't have to import `destroy()` right? My nit-pick is that destroy resets to .init after destruction, which makes it feel like more than a destroy, and that is almost certainly the reason people reach for __xdtor() when they do (ie, they JUST want to destruct). We should probably address that, and then publish a strong recommendation.
May 23 2018
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/23/18 1:41 PM, Manu wrote:
 On 23 May 2018 at 05:38, Steven Schveighoffer via Digitalmars-d
 <digitalmars-d puremagic.com> wrote:
 On 5/22/18 10:15 PM, Manu wrote:
 If you're in the mood to prepare a DIP, I think you should prepare this
 dip:

 Support the syntax:
     classInstance.~this();
Isn't this just classInstance.destroy() ?
Is everything in object.d globally available? You don't have to import `destroy()` right? My nit-pick is that destroy resets to .init after destruction, which makes it feel like more than a destroy, and that is almost certainly the reason people reach for __xdtor() when they do (ie, they JUST want to destruct). We should probably address that, and then publish a strong recommendation.
I think it's a good idea. Like unsafeDestroy. -Steve
May 23 2018
parent reply Manu <turkeyman gmail.com> writes:
On 23 May 2018 at 10:59, Steven Schveighoffer via Digitalmars-d
<digitalmars-d puremagic.com> wrote:
 On 5/23/18 1:41 PM, Manu wrote:
 On 23 May 2018 at 05:38, Steven Schveighoffer via Digitalmars-d
 <digitalmars-d puremagic.com> wrote:
 On 5/22/18 10:15 PM, Manu wrote:
 If you're in the mood to prepare a DIP, I think you should prepare this
 dip:

 Support the syntax:
     classInstance.~this();
Isn't this just classInstance.destroy() ?
Is everything in object.d globally available? You don't have to import `destroy()` right? My nit-pick is that destroy resets to .init after destruction, which makes it feel like more than a destroy, and that is almost certainly the reason people reach for __xdtor() when they do (ie, they JUST want to destruct). We should probably address that, and then publish a strong recommendation.
I think it's a good idea. Like unsafeDestroy.
I hate the name. People will constantly reach for the wrong thing. Such a long name says to me "this isn't the function you're looking for" in terms of intuition. For my money it would be: destroy() <- just destroy reset() <- destroy and re-init Or something like that. Maybe: class.destruct(); <- destroy without init? Yeah that. I'll make a PR!
May 23 2018
parent Mike Franklin <slavo5150 yahoo.com> writes:
On Wednesday, 23 May 2018 at 19:05:38 UTC, Manu wrote:

 For my money it would be:
 destroy() <- just destroy
 reset() <- destroy and re-init

 Or something like that.

 Maybe:
 class.destruct(); <- destroy without init?

 Yeah that. I'll make a PR!
I had an in-depth discussion with Andrei about this at https://github.com/dlang/druntime/pull/2115 He ultimately gave up on trying to explain it to me and created his own PR here: https://github.com/dlang/druntime/pull/2126 It's be good to review that discussion, and perhaps weigh in on Mike
May 27 2018
prev sibling parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/23/18 1:41 PM, Manu wrote:
 Is everything in object.d globally available? You don't have to import
 `destroy()` right?
Oh, didn't answer this, yes object.d is always imported. -Steve
May 23 2018
prev sibling next sibling parent Guillaume Piolat <first.last gmail.com> writes:
On Wednesday, 23 May 2018 at 01:59:50 UTC, sarn wrote:
 I'm starting this thread to talk about ways to make things 
 better, but first the bad news.  Let's use this idiomatic code 
 as an example of typical bugs:
Keep up the good work of explaining surprising things! Someone has to do Scott's job for D.
May 23 2018
prev sibling next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/22/18 9:59 PM, sarn wrote:
 (I'm referring to Scott's 2014 DConf talk: 
 https://www.youtube.com/watch?v=KAWA1DuvCnQ)
 
 I was actually preparing a DIP about this when Manu posted to the forums 
 about his own related problems with C++ interop.
 
 I traced a bug in some of my D code to my own misunderstanding of how 
 D's destructors actually work.  So I did some research and discovered a 
 bunch of edge cases with using __dtor, __xdtor and 
 hasElaborateDestructor.  I tried reviewing the packages on 
 code.dlang.org and some elsewhere (thankfully only about 1% of D 
 packages use these functions directly) and it turns out I'm not the only 
 one confused.  I don't have time to file bug reports for every package, 
 so, if you're responsible for code that handles destructors manually, 
 please do a review.  There's a *lot* of buggy code out there.
 
I think it's very good to bring attention to these pitfalls, thanks!
 
 Here are some recommendations:
 * Really try to just use destroy().  Manually working with 
 __dtor/__xdtor is a minefield, and I haven't seen any code that actually 
 reimplements the RTTI walk that the runtime library does. 
This is advice you need to follow. Using the underlying __functions are error prone, and subject to weird errors as you have found. The other thing to do is to follow the example of what Phobos functions do when dealing with these low-level functions. In terms of classes, the RTTI walk is callable via the function _rt_finalize (an extern(C) function, you can prototype it anywhere). I highly recommend just calling destroy as it will do this.
 (Unfortunately destroy() currently isn't zero-overhead for plain old 
 data structs because it's based on RTTI, but at least it works.)
Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs. Please file an enhancement request.
 * Avoid trying to figure out if something needs destruction. Just 
 destroy everything, or make it clear you don't own classes at all, or be 
 totally sure you're working with plain old data structs.
Ideally, destroy should do the most efficient thing. So this is good advice as well.
 * Some code uses __dtor as a way to manually run cleanup code on an 
 object that will be used again.  Putting this cleanup code into a normal 
 method will cause fewer headaches.
Using __dtor is a very bad idea in almost all cases. Putting cleanup code into a normal function can have some of the same pitfalls, however (what if you forget to call the super version of the method?). The only *correct* way to destroy an object is to follow the runtime's example or call the function directly. The destructor also has the nice feature of being called when the struct goes out of scope. Best advice -- just use destroy on types to clean them up.
 
 The one other usage of these low-level destructor facilities is checking 
 if a type is a plain old data struct.  This is an important special case 
 for some code, but everyone seems to do the check a different way.  
 Maybe a special isPod trait is needed.
 
isPod as you have described it is not difficult: enum isPod(T) = is(T == struct) && !hasElaborateDestructor!T; But this is not necessarily the definition of POD. Generally this means it has no postblit, and some people may even be expecting such a thing to have no methods as well. So I'm not sure we want to add such a definition to the library. Clearly destroy is preferred to checking or doing anything else, but maybe in some cases we want a more efficient destruction that cuts corners. For instance, there's no reason to blit the init value back to the object when its memory is being reclaimed. That building block is somewhat inextricable from destroy at the moment. Ideally, you shouldn't have to worry about it as a normal user -- some genius library author should take care of these details. Unfortunately, the only place where this happens is on the stack or in the GC. All other memory allocation schemes require you to get your hands dirty with destruction. -Steve
May 23 2018
next sibling parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
 On 5/22/18 9:59 PM, sarn wrote:
 (Unfortunately destroy() currently isn't zero-overhead for plain old 
 data structs because it's based on RTTI, but at least it works.)
Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs.
Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178
 Please file an enhancement request.
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
May 23 2018
next sibling parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Wednesday, 23 May 2018 at 15:43:31 UTC, Steven Schveighoffer 
wrote:
 On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
 On 5/22/18 9:59 PM, sarn wrote:
 (Unfortunately destroy() currently isn't zero-overhead for 
 plain old data structs because it's based on RTTI, but at 
 least it works.)
Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs.
Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178
 Please file an enhancement request.
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
The destroy function for class/structs badly need an overhaul. Even more so for class, as (IMO) it should be responsibility of the class designer when it comes to deinitializing (The exception being that creating the class exclusively for the GC to use of course), as finalize function which destroy calls is external, in which information such as attributes is lost
May 23 2018
parent reply Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
On Wednesday, May 23, 2018 17:29:11 12345swordy via Digitalmars-d wrote:
 On Wednesday, 23 May 2018 at 15:43:31 UTC, Steven Schveighoffer

 wrote:
 On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
 On 5/22/18 9:59 PM, sarn wrote:
 (Unfortunately destroy() currently isn't zero-overhead for
 plain old data structs because it's based on RTTI, but at
 least it works.)
Hm.. that should be fixed. I don't see why we can't just do = T.init, we should at least be optimizing to this for small enough structs.
Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178
 Please file an enhancement request.
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
The destroy function for class/structs badly need an overhaul. Even more so for class, as (IMO) it should be responsibility of the class designer when it comes to deinitializing (The exception being that creating the class exclusively for the GC to use of course), as finalize function which destroy calls is external, in which information such as attributes is lost
Regardless of what issues destroy may currently have, the entire reason that it exists is to destroy objects - and class objects in particular. So, if it doesn't currently work properly for that purpose, it needs to be fixed. - Jonathan M Davis
May 23 2018
parent reply 12345swordy <alexanderheistermann gmail.com> writes:
On Wednesday, 23 May 2018 at 17:47:12 UTC, Jonathan M Davis wrote:
 On Wednesday, May 23, 2018 17:29:11 12345swordy via 
 Digitalmars-d wrote:
 On Wednesday, 23 May 2018 at 15:43:31 UTC, Steven Schveighoffer

 wrote:
 On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
 [...]
Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178
 [...]
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
The destroy function for class/structs badly need an overhaul. Even more so for class, as (IMO) it should be responsibility of the class designer when it comes to deinitializing (The exception being that creating the class exclusively for the GC to use of course), as finalize function which destroy calls is external, in which information such as attributes is lost
Regardless of what issues destroy may currently have, the entire reason that it exists is to destroy objects - and class objects in particular. So, if it doesn't currently work properly for that purpose, it needs to be fixed. - Jonathan M Davis
Here is the thing though, when you call destroy on the object, you would assume by the name that the memory occupied by it will be deallocated which is NOT always the case. At this point I rather rename it as .deint, as it is less misleading here.
May 23 2018
parent reply Jonathan M Davis <newsgroup.d jmdavisprog.com> writes:
On Wednesday, May 23, 2018 18:04:28 12345swordy via Digitalmars-d wrote:
 On Wednesday, 23 May 2018 at 17:47:12 UTC, Jonathan M Davis wrote:
 On Wednesday, May 23, 2018 17:29:11 12345swordy via

 Digitalmars-d wrote:
 On Wednesday, 23 May 2018 at 15:43:31 UTC, Steven Schveighoffer

 wrote:
 On 5/23/18 9:12 AM, Steven Schveighoffer wrote:
 [...]
Coincidentally, this JUST changed due to a different reason: https://github.com/dlang/druntime/pull/2178
 [...]
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
The destroy function for class/structs badly need an overhaul. Even more so for class, as (IMO) it should be responsibility of the class designer when it comes to deinitializing (The exception being that creating the class exclusively for the GC to use of course), as finalize function which destroy calls is external, in which information such as attributes is lost
Regardless of what issues destroy may currently have, the entire reason that it exists is to destroy objects - and class objects in particular. So, if it doesn't currently work properly for that purpose, it needs to be fixed. - Jonathan M Davis
Here is the thing though, when you call destroy on the object, you would assume by the name that the memory occupied by it will be deallocated which is NOT always the case. At this point I rather rename it as .deint, as it is less misleading here.
The documentation is quite clear that it does not free the memory, and it's never been the case that it was supposed to. And we already renamed it once from clear because of the problems that it caused with containers. At this point, I think that it would be overkill to rename it yet again, especially since we've been trying to not rename stuff unless we really need to, because it causes code breakage for what it usually minimal benefit. - Jonathan M Davis
May 23 2018
parent 12345swordy <alexanderheistermann gmail.com> writes:
On Wednesday, 23 May 2018 at 18:11:45 UTC, Jonathan M Davis wrote:
 On Wednesday, May 23, 2018 18:04:28 12345swordy via 
 Digitalmars-d wrote:
 On Wednesday, 23 May 2018 at 17:47:12 UTC, Jonathan M Davis 
 wrote:
 On Wednesday, May 23, 2018 17:29:11 12345swordy via

 Digitalmars-d wrote:
 [...]
Regardless of what issues destroy may currently have, the entire reason that it exists is to destroy objects - and class objects in particular. So, if it doesn't currently work properly for that purpose, it needs to be fixed. - Jonathan M Davis
Here is the thing though, when you call destroy on the object, you would assume by the name that the memory occupied by it will be deallocated which is NOT always the case. At this point I rather rename it as .deint, as it is less misleading here.
The documentation is quite clear that it does not free the memory, and it's never been the case that it was supposed to. And we already renamed it once from clear because of the problems that it caused with containers. At this point, I think that it would be overkill to rename it yet again, especially since we've been trying to not rename stuff unless we really need to, because it causes code breakage for what it usually minimal benefit. - Jonathan M Davis
My point being is that current objects rely on the external finalize function for de-inialtalzation, which is not ideal if you wanted to call destroy in the scope of system default attributes like safe or nogc. How do you think that the finalize function should behave in these contexts?
May 23 2018
prev sibling parent sarn <sarn theartofmachinery.com> writes:
On Wednesday, 23 May 2018 at 15:43:31 UTC, Steven Schveighoffer 
wrote:
 Coincidentally, this JUST changed due to a different reason: 
 https://github.com/dlang/druntime/pull/2178

 Please file an enhancement request.
I still think it could be better, so I added a further issue: https://issues.dlang.org/show_bug.cgi?id=18899 -Steve
This is really good news!
May 23 2018
prev sibling next sibling parent reply sarn <sarn theartofmachinery.com> writes:
On Wednesday, 23 May 2018 at 13:12:57 UTC, Steven Schveighoffer 
wrote:
 On 5/22/18 9:59 PM, sarn wrote:
 * Some code uses __dtor as a way to manually run cleanup code 
 on an object that will be used again.  Putting this cleanup 
 code into a normal method will cause fewer headaches.
Using __dtor is a very bad idea in almost all cases. Putting cleanup code into a normal function can have some of the same pitfalls, however (what if you forget to call the super version of the method?). The only *correct* way to destroy an object is to follow the runtime's example or call the function directly. The destructor also has the nice feature of being called when the struct goes out of scope. Best advice -- just use destroy on types to clean them up.
Here's an example of what I'm talking about: https://github.com/dlang/phobos/blob/master/std/signals.d#L230 It's running __dtor manually just to run some code that happens to be in the destructor. It's obviously not meant to run any other destructors (at least, the documentation doesn't say "Use this mixin in your object and then calling disconnectAll() will destroy everything in your object."). It's a broken design pattern, but existing code is doing it. (As I said, I reviewed a lot of D packages, and I don't have time to file bug reports or PRs for each one.)
 But this is not necessarily the definition of POD. Generally 
 this means it has no postblit, and some people may even be 
 expecting such a thing to have no methods as well. So I'm not 
 sure we want to add such a definition to the library.
The common case is that some data types can be blitted around as raw memory without worrying about destructors, postblits, or whatever is added to the language in future. This is the thing that seems to matter. (Have you seen any code that needs to care if a struct has methods? It sounds like a very special case that can still be handled using current compile-time reflection anyway.) __traits(isPOD) seems to do the job, and is a lot better than the ad hoc implementations I've seen. We should encourage people to use it more often.
May 23 2018
parent reply Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/23/18 7:11 PM, sarn wrote:
 On Wednesday, 23 May 2018 at 13:12:57 UTC, Steven Schveighoffer wrote:
 On 5/22/18 9:59 PM, sarn wrote:
 * Some code uses __dtor as a way to manually run cleanup code on an 
 object that will be used again.  Putting this cleanup code into a 
 normal method will cause fewer headaches.
Using __dtor is a very bad idea in almost all cases. Putting cleanup code into a normal function can have some of the same pitfalls, however (what if you forget to call the super version of the method?). The only *correct* way to destroy an object is to follow the runtime's example or call the function directly. The destructor also has the nice feature of being called when the struct goes out of scope. Best advice -- just use destroy on types to clean them up.
Here's an example of what I'm talking about: https://github.com/dlang/phobos/blob/master/std/signals.d#L230 It's running __dtor manually just to run some code that happens to be in the destructor.  It's obviously not meant to run any other destructors (at least, the documentation doesn't say "Use this mixin in your object and then calling disconnectAll() will destroy everything in your object.").
This is a bug. That module is not well-used and has not received a lot of attention, if you look at the development history, almost all changes are global style changes.
 
 It's a broken design pattern, but existing code is doing it.  (As I 
 said, I reviewed a lot of D packages, and I don't have time to file bug 
 reports or PRs for each one.)
Understood. I'll file one for you on this one. It's one thing to have some random package using an incorrect pattern, it's another thing to have Phobos doing it.
 But this is not necessarily the definition of POD. Generally this 
 means it has no postblit, and some people may even be expecting such a 
 thing to have no methods as well. So I'm not sure we want to add such 
 a definition to the library.
The common case is that some data types can be blitted around as raw memory without worrying about destructors, postblits, or whatever is added to the language in future.  This is the thing that seems to matter.  (Have you seen any code that needs to care if a struct has methods?  It sounds like a very special case that can still be handled using current compile-time reflection anyway.)
 __traits(isPOD) seems to do the job, and is a lot better than the ad hoc 
 implementations I've seen.  We should encourage people to use it more 
 often.
From the D spec, POD means a "struct that contains no hidden members, does not have virtual functions, does not inherit, has no destructor, and can be initialized and copied via simple bit copies" https://dlang.org/glossary.html#pod Which is what __traits(isPOD) requires. This seems like what everyone should use when looking for POD, I wasn't aware we had a __traits for it. -Steve
May 24 2018
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/24/18 5:32 AM, Steven Schveighoffer wrote:
 On 5/23/18 7:11 PM, sarn wrote:
 Here's an example of what I'm talking about:
 https://github.com/dlang/phobos/blob/master/std/signals.d#L230

 It's running __dtor manually just to run some code that happens to be 
 in the destructor.  It's obviously not meant to run any other 
 destructors (at least, the documentation doesn't say "Use this mixin 
 in your object and then calling disconnectAll() will destroy 
 everything in your object.").
This is a bug. That module is not well-used and has not received a lot of attention, if you look at the development history, almost all changes are global style changes.
 It's a broken design pattern, but existing code is doing it.  (As I 
 said, I reviewed a lot of D packages, and I don't have time to file 
 bug reports or PRs for each one.)
Understood. I'll file one for you on this one. It's one thing to have some random package using an incorrect pattern, it's another thing to have Phobos doing it.
Wow, so I learned a couple things reading that code. 1. Yes you are right, it's only meant to call the local mixin destructor, not any other destructors (it doesn't even call the destructor of the class it's mixed into). This "valid" use of __dtor has super-huge code smell. 2. I didn't realize that you could mixin extra pieces to a destructor! I added the bug report here: https://issues.dlang.org/show_bug.cgi?id=18903. I outline exactly how to fix it, if anyone wants an easy PR opportunity. -Steve
May 24 2018
prev sibling parent reply Mike Franklin <slavo5150 yahoo.com> writes:
On Wednesday, 23 May 2018 at 13:12:57 UTC, Steven Schveighoffer 
wrote:

 Hm.. that should be fixed. I don't see why we can't just do = 
 T.init, we should at least be optimizing to this for small 
 enough structs.
The problem I ran into with this is that if you use T.init directly you end up making copies (postblit) and then needing to destruct those copies since they are temporary. That's why I had to lower it to simpler byte array primitive. Mike
May 23 2018
parent Steven Schveighoffer <schveiguy yahoo.com> writes:
On 5/23/18 9:38 PM, Mike Franklin wrote:
 On Wednesday, 23 May 2018 at 13:12:57 UTC, Steven Schveighoffer wrote:
 
 Hm.. that should be fixed. I don't see why we can't just do = T.init, 
 we should at least be optimizing to this for small enough structs.
The problem I ran into with this is that if you use T.init directly you end up making copies (postblit) and then needing to destruct those copies since they are temporary.  That's why I had to lower it to simpler byte array primitive.
This is the fool-proof way, but this is a template function! We can deduce whether doing the T.init copy is going to call a postblit or destructor. -Steve
May 24 2018
prev sibling next sibling parent reply Paul Backus <snarwin gmail.com> writes:
On Wednesday, 23 May 2018 at 01:59:50 UTC, sarn wrote:
 The one other usage of these low-level destructor facilities is 
 checking if a type is a plain old data struct.  This is an 
 important special case for some code, but everyone seems to do 
 the check a different way.  Maybe a special isPod trait is 
 needed.
There's this: https://dlang.org/spec/traits.html#isPOD
May 23 2018
parent sarn <sarn theartofmachinery.com> writes:
On Wednesday, 23 May 2018 at 19:28:28 UTC, Paul Backus wrote:
 On Wednesday, 23 May 2018 at 01:59:50 UTC, sarn wrote:
 The one other usage of these low-level destructor facilities 
 is checking if a type is a plain old data struct.  This is an 
 important special case for some code, but everyone seems to do 
 the check a different way.  Maybe a special isPod trait is 
 needed.
There's this: https://dlang.org/spec/traits.html#isPOD
Damn, that's exactly what I wanted; I don't know how I missed it. I guess that's why it's good to bounce ideas off others.
May 23 2018
prev sibling parent reply Mike Franklin <slavo5150 yahoo.com> writes:
On Wednesday, 23 May 2018 at 01:59:50 UTC, sarn wrote:
 (I'm referring to Scott's 2014 DConf talk: 
 https://www.youtube.com/watch?v=KAWA1DuvCnQ)

 I was actually preparing a DIP about this when Manu posted to 
 the forums about his own related problems with C++ interop.

 I traced a bug in some of my D code to my own misunderstanding 
 of how D's destructors actually work.  So I did some research 
 and discovered a bunch of edge cases with using __dtor, __xdtor 
 and hasElaborateDestructor.  I tried reviewing the packages on 
 code.dlang.org and some elsewhere (thankfully only about 1% of 
 D packages use these functions directly) and it turns out I'm 
 not the only one confused.  I don't have time to file bug 
 reports for every package, so, if you're responsible for code 
 that handles destructors manually, please do a review.  There's 
 a *lot* of buggy code out there.

 I'm starting this thread to talk about ways to make things 
 better, but first the bad news.  Let's use this idiomatic code 
 as an example of typical bugs:
In the context of your blog post at https://theartofmachinery.com/2018/05/27/cpp_classes_in_betterc.html, I'm wondering if the changes in 2.081 help matters at all: I'm wondering if any of the changes in 2.081 improves the situation here: e.g. https://dlang.org/changelog/2.081.0.html#cpp_destroy Mike
Jul 08 2018
parent reply sarn <sarn theartofmachinery.com> writes:
On Monday, 9 July 2018 at 01:19:28 UTC, Mike Franklin wrote:
 In the context of your blog post at 
 https://theartofmachinery.com/2018/05/27/cpp_classes_in_betterc.html, I'm
wondering if the changes in 2.081 help matters at all:  I'm wondering if any of
the changes in 2.081 improves the situation here:  e.g.
https://dlang.org/changelog/2.081.0.html#cpp_destroy

 Mike
I don't know yet, but, yeah, that work looks interesting and I definitely have to do some more testing. BTW, I'm still interested in writing DIPs. I've just been working a lot on another project lately.
Jul 08 2018
parent 12345swordy <alexanderheistermann gmail.com> writes:
On Monday, 9 July 2018 at 02:10:27 UTC, sarn wrote:
 On Monday, 9 July 2018 at 01:19:28 UTC, Mike Franklin wrote:
 In the context of your blog post at 
 https://theartofmachinery.com/2018/05/27/cpp_classes_in_betterc.html, I'm
wondering if the changes in 2.081 help matters at all:  I'm wondering if any of
the changes in 2.081 improves the situation here:  e.g.
https://dlang.org/changelog/2.081.0.html#cpp_destroy

 Mike
I don't know yet, but, yeah, that work looks interesting and I definitely have to do some more testing. BTW, I'm still interested in writing DIPs. I've just been working a lot on another project lately.
I was thinking of rewriting my current DIP into introducing null function and recursive_destruct that can call on classes only if certain rules are checked. -Alexander
Jul 08 2018