Bug 17897 – Incorrect number of destructor calls in example

Test case: import std.stdio; struct Foo { this(int) {} ~this() {} } struct Bar { this(this) { writefln("Bar.this(this): %X", &this); } this(Foo, Foo) { writefln("Bar.this(int): %X", &this); } ~this() { writefln("Bar.~this(): %X", &this); } } void fun(Bar n) { writefln("fun: %X", &n); } void main() { Foo foo; fun(Bar(foo, Foo(0))); } Result: Bar.this(int): XXXXXXXXXXXX fun: XXXXXXXXXXXX Bar.~this(): XXXXXXXXXXXX Bar.~this(): XXXXXXXXXXXX Expected: Bar.this(int): XXXXXXXXXXXX Bar.this(this): XXXXXXXXXXXX fun: XXXXXXXXXXXX Bar.~this(): XXXXXXXXXXXX Bar.~this(): XXXXXXXXXXXX Discussion: https://forum.dlang.org/post/[email protected]

Postblit is not called for moves.

Sorry, I didn't mean to commit so early. A postblit is not called for a move, which is done for rvalues being sent into a function: import std.stdio; struct S { this(this) { writeln("postblit"); } } void foo(S s) { } void main() { foo(S()); // no postblit, this is an rvalue. The struct isn't actually moved anyway. S s; foo(s); // postblit called, because we made a copy of s onto the stack to send into foo. }

(In reply to Steven Schveighoffer from comment #2) > Sorry, I didn't mean to commit so early. > > A postblit is not called for a move, which is done for rvalues being sent > into a function: snip Except this isn't just a move. No, the number of destructor calls must be equal to the number of constructor/postblit calls. Otherwise reference counting types are not going to work. Postblit must be called in the given example. Count: 1 - create Count: 1 - fun call Count: 0 - destructor (deallocate memory) Count: segfault, memory has already been freed - destructor (deallocate memory) Something is still wrong in the given example.

(In reply to Steven Schveighoffer from comment #1) > Postblit is not called for moves. You're misreading the bug report (not surprising, as it was marked incorrectly). Expanded example: import std.stdio; struct Foo { this(int) {} ~this() {} } struct Baz {} struct Bar(T) { this(T a) { writefln("Bar.this(T): %X", &this); } this(T a, T b) { writefln("Bar.this(T, T): %X", &this); } ~this() { writefln("Bar.~this(): %X", &this); } } void fun(T)(Bar!T n) { writefln("fun: %X", &n); } unittest { // 1 // Basically what Jack posted: int n; writefln("\nunittest 1: %X", &n); fun(Bar!Foo(Foo(0), Foo(0))); } unittest { // 2 // Single Foo in Bar's constructor: int n; writefln("\nunittest 2: %X", &n); fun(Bar!Foo(Foo(0))); } unittest { // 3 // Saving Foo(0) to a temporary: int n; writefln("\nunittest 3: %X", &n); auto foo = Foo(0); fun(Bar!Foo(foo, foo)); } unittest { // 4 // using a type without constructor and destructor: int n; writefln("\nunittest 4: %X", &n); fun(Bar!Baz(Baz(), Baz())); } Which gives this output: unittest 1: 19FDAC Bar.this(T, T): 19FDB3 fun: 19FD80 Bar.~this(): 19FD80 Bar.~this(): 19FDB3 unittest 2: 19FDB4 Bar.this(T): 19FDB8 fun: 19FD8C Bar.~this(): 19FD8C unittest 3: 19FDA8 Bar.this(T, T): 19FDAD fun: 19FD7C Bar.~this(): 19FD7C unittest 4: 19FDB8 Bar.this(T, T): 19FDBC fun: 19FD94 Bar.~this(): 19FD94 As we can see for unittest1, the destructor is called twice - once for the temporary in the unittest block, and once when we exit fun(). This is caused by some interaction with Foo's constructor and destructor, as evinced by the fact that commenting out either fixes the problem (see unittest 4). Other things that fixes it include changing Bar's constructor to take a single Foo instead of two (as seen in unittest 2), and saving Foo(0) to a temporary before passing it to Bar() (as seen in unittest 3).

The bug in the example is that the destructor is called twice, not that postblit is not called. I assumed you were going to file a bug on the destructor calls, but looks like this is it, so I'll just change the title. (In reply to Simen Kjaeraas from comment #4) > (In reply to Steven Schveighoffer from comment #1) > As we can see for unittest1, the destructor is called twice - once for the > temporary in the unittest block, and once when we exit fun(). No, it's called once for the local in fun, and another time for no reason (an actual bug). It's still a move.

Expected should read: Bar.this(int): XXXXXXXXXXXX fun: XXXXXXXXXXXX Bar.~this(): XXXXXXXXXXXX

(In reply to Steven Schveighoffer from comment #5) > The bug in the example is that the destructor is called twice, not that > postblit is not called. Not exactly. In fact, it is important that the number of constructor calls was equal to the number of destructors calls (as Richard said). So this bug can be interpreted as missed postblit or extra destructor. I believe that the compiler is free to choose to make a copy or not. Or am I mistaken and the exact behavior is somewhere in specs?

I don't know if it's in the spec or not, but clearly, there is no reason for the temporary not to be constructed in-place on the stack for passage into fun. It does not need to construct it and then make a copy, that would be wasteful. Yes, the ultimate guarantee is that the number of postblits/ctor calls must equal the number of dtor calls. But the bug is definitely that an extra dtor call is made. Compiling with -vcg-ast shows no such call, I only see one. The extra dtor is completely erroneous.

Seems this bug related to https://issues.dlang.org/show_bug.cgi?id=17246

After fixing https://issues.dlang.org/show_bug.cgi?id=17246 this outputs: Bar.this(int): 14FB84 fun: 14FB20 Bar.~this(): 14FB20 Object gets moved without notification. This should not behave this way

(In reply to Temtaime from comment #10) > After fixing https://issues.dlang.org/show_bug.cgi?id=17246 this outputs: > > Bar.this(int): 14FB84 > fun: 14FB20 > Bar.~this(): 14FB20 > > Object gets moved without notification. > This should not behave this way This is perfectly valid. You are allowed to move a struct without calling the postblit. See the spec here: https://dlang.org/spec/struct.html "A struct is defined to not have an identity; that is, the implementation is free to make bit copies of the struct as convenient." In fact, I think we can close this as a duplicate, as it now works properly. However, I'm kind of surprised there is a move here, should be unnecessary if implemented properly.

Closing as duplicate *** This issue has been marked as a duplicate of issue 17246 ***