Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Let's solve object.destroy problem #344

Closed
wants to merge 3 commits into from

Conversation

denis-sh
Copy link
Contributor

@denis-sh denis-sh commented Nov 5, 2012

[EDITED] 3 times

Let us have finalizeClassInstance (a function finalizing a class instance referenced by its argument) and destruct (a function destroying its argument equivalent as if it goes out of scope). Combining these two functions in one (current destroy) is needless, error-prone, and inconsistent.

It is needless because having one name assumes is is used in templated code but there is no common uses for such combined function.

It is error-prone because e.g. when destroy is used in templated code like creating an analog of a struct with manual memory management:

void destructIt()
{
    ...
    foreach(i, T; Types)
        destroy(* cast(T*) getMemory(i));
    ...
}

it is easy to forget about the fact that destroy behaves different for class references. As this is the common situation, destroy require a redundant static if branch in the bast case and causes nasty bugs in the worst.

It is inconsistent, because druntime isn't a place for template code at all and really fast and CTFE-able destructing function use templates heavily (see destruct proposal for example).


This is a good time to deprecate object.destroy because:

  • it has never been properly documented (yes, one does know nothing about how it really works for specific types without reading its sources)
  • it has never worked correctly for static arrays
  • it has never worked for shared structs

And yes, current destroy is just as bad as if we were have .sizeof property returning for classes its instance size instead of __traits(classInstanceSize, ...).


See also:

Pull dlang/phobos#929 with destruct proposal.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 6, 2012

Wow! Even in my object.destroy investigation I completely missed that it doesn't nullify class reference as it event doesn't accept it by ref. Updated.

{
enum bool _isStaticArray = true;
}
static if(is(T == struct) || is(typeof(typeid(T)) == TypeInfo_StaticArray))
Copy link
Member

Choose a reason for hiding this comment

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

Will this work for e.g. const(int)[32]? And for that matter, did the old code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither new nor old code works for const.

It looks like it's impossible to work correct with shared because of this:

int i = 0;

struct S
{
    ~this ()
    { ++i; }
}

struct SS
{
    shared ~this ()
    { ++i; }
}

void main()
{
    S s;
    typeid(S).destroy(&s);
    assert(i == 1); i = 0; // ok
    typeid(shared S).destroy(&s);
    assert(i == 1); i = 0; // failed

    // can't use S here because of compiler bug (but can use in `shared S[1] sarr`)
    shared SS ss;
    typeid(SS).destroy(cast(void*) &ss);
    assert(i == 1); i = 0; // ok
    typeid(shared SS).destroy(cast(void*) &ss);
    assert(i == 1); i = 0; // failed
}

Anyway, I completely forget about shared. Will change to mimic old destroy shared-rejecting behavior.

@alexrp
Copy link
Member

alexrp commented Nov 6, 2012

I don't see any compelling reason to rename this function to finalizeClassInstance? Am I missing something?

@alexrp
Copy link
Member

alexrp commented Nov 6, 2012

Also, I think these functions should take their arguments as auto ref and, if the argument is assignable, set it to T.init.

@jmdavis
Copy link
Member

jmdavis commented Nov 6, 2012

Let's leave the name as it is but deprecate the non-Object overloads?

Why would we make it not work for other types? If it needs further tweaking to work appropriately for them, then fine, but I see no reason to remove the functionality.

@alexrp
Copy link
Member

alexrp commented Nov 6, 2012

Why would we make it not work for other types? If it needs further tweaking to work appropriately for them, then fine, but I see no reason to remove the functionality.

I deleted that comment. I misunderstood what he was trying to do.

@jmdavis
Copy link
Member

jmdavis commented Nov 6, 2012

So, you're trying to make it so that destroy doesn't work for classes, and you use finalizeClassInstance instead? Why rename it? We just renamed it from clear. I see no point in renaming it again, and giving it a different name for different types is just confusing. If we need to fix its functionality, fine, but don't change the function's name.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 7, 2012

Added static asserts to destroy in the first commit to properly emulate old version with unittets.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 7, 2012

Also, I think these functions should take their arguments as auto ref and, if the argument is assignable, set it to T.init.

As object.destroy is for backward compatibility only, no braking changes except fixing incorrect behavior here. Lets discuss it in my destruct pull dlang/phobos#929.

For 2 comments with:

... finalizeClassInstance ... ?

You do not understand me, I do not understand you. Looks like your comments means:

We see all your reasons and they are so incorrect that we even will not bother to write you why they are incorrect.

What can I answer then? Probably I can give you code example:

import std.typecons: destruct;

class C { ... }

void main()
{
    C c = new C(...);
    destroy(c);   // to call runtime finalize
    destruct(c);  // to clear the reference
    assert(c is null); // be happy
}

So having both destroy and destruct looks incredibly confusing.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 7, 2012

Edited description to make clearer Now the worst thing with object.destroy section.

@alexrp
Copy link
Member

alexrp commented Nov 7, 2012

We see all your reasons and they are so incorrect that we even will not bother to write you why they are incorrect.

Dude, no. What it means is that it wasn't clear to us why a rename was warranted. That just means we wanted clarification.

Let us step back for a bit.

How about we just make destroy in druntime do the right thing for all types (run destructor and so on depending on the type), and nullify/reset the destroyed variable if and only if a new, optional bool reset = false parameter is true? For this scheme to work, you'd need to take the variable to be destroyed as auto ref such that it works for both lvalues and rvalues.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 7, 2012

Dude, no. What it means is that it wasn't clear to us why a rename was warranted. That just means we wanted clarification.

I clarified as clear as possible. It is in description.

How about we just make destroy in druntime do the right thing for all types (run destructor and so on depending on the type), and nullify/reset the destroyed variable if and only if a new, optional bool reset = false parameter is true?

Now the worst thing with object.destroy section tells about 'correct' destroy behavior that is inconsistent. It's a generic code killer to not know does destroy(t) really destroy t or something t references to and it is so hard to remember and so easy to do a mistake that I really wan some arguments from you why it showld be rest as is.

For this scheme to work, you'd need to take the variable to be destroyed as auto ref such that it works for both lvalues and rvalues.

As I'm still sure we need finalizeClassInstance, there is no ref problems I can see with destruct from my other pull. But Jonathan M Davis still trying to say that there is some problems. Can you, please, write you opinion in our dlang/phobos#929 discussion?

@jmdavis
Copy link
Member

jmdavis commented Nov 7, 2012

  1. Why not just fix destroy rather than creating a new function? What does the new function buy you?
  2. Why have you created destruct in std.typecons? It's basically doing what destroy is supposed to do.

And actually, from your description, I'd say that destroy is consistent without your changes. It's destroying the object that you give it and never following an indirections.

For a class, that's the object that you give it. As far as the type system is concerned, the reference is the class. There is no separation of the reference and the class. You can't dereference it or otherwise indicate that you want to do something to the object rather than the reference. So, calling destroy on the class naturally destroys the object. Its finalizer is called, and the destructors of any value types that it contains are called. Any reference types that it refers to are left alone.

For pointers, there is a separation between the pointer and the type being pointed to. So, having destroy destroy the pointer rather than the object pointed to makes sense. If you want to destroy the object pointed to, then dereference the pointer and call destroy on that.

For structs, there's no indirection of any kind involved. Destroying the object should call its destructor and set it to its init value. The result should be identical to what happens when you pass a struct to an out parameter. So, any other value types contained by that struct should be destroyed as well, but any reference types will then simply not be referenced anymore. No class objects or dynamic arrays should be destroyed. Just structs, static arrays, and the integral/floating/boolean/char types should be destroyed.

For dynamic arrays, they're pointers with length, so it should be essentially the same as calling destroy on a pointer except that length gets zeroed out as well.

For static arrays, like structs, there's no indirection, so it's basically the same. Any value types that they hold should have their destructors called and be set to init (which means setting the whole static array to its init value).

What you're trying to do seems to be to have destroy operate not just on what was passed to it but also everything that it indirectly refers to. That's a huge change to how it functions. It's fundamentally different, and it makes it far more dangerous, especially when you consider classes which hold other classes. There's a huge difference between destroying an object and destroying everything that it refers to.

It seems to me that the problem is that you have a fundamental misunderstanding of destroy rather than there being anything actually wrong with it. destroy is essentially the same as calling the destructor on something and setting it to its init value. It's what happens with objects passed to out parameters. What you're trying to make it do is very different.

@jmdavis
Copy link
Member

jmdavis commented Nov 7, 2012

Actually, I should correct myself. The out example isn't quite right. out simply sets the variable to its init value, which doesn't necessarily involve any destruction, whereas destroy specifically destroys the object before setting the object to its init value.

Regardless, as far as I can tell, destroy is consistent as far as I can tell. It operates on what you pass it and not what that refers to. This seems a bit off when it comes to classes simply because the type of the class is not separate from the type of the reference, so there's no way to indicate that you want to destroy the class object rather than simply setting the reference to null, whereas dynamic arrays and pointers do have that separation, so it makes sense to make them destroy the pointer rather than what's pointed to just like occurs with structs and static arrays. It's the same as how putting const on a reference can't refer simply to the reference but it can with pointers.

@ghost
Copy link

ghost commented Nov 7, 2012

Semantic changes like in this pull need to be discussed in the newsgroup first, not split out across multiple pull requests.

buf[] = 0;
else
{
TypeInfo el = typeid(T), info = el;
Copy link

Choose a reason for hiding this comment

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

Don't depend on the order of evaluation of arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Order of evaluation of initializations is defined as left to right. Never heard about compiler bugs in this area. List it here if any.
By the way, order of evaluation of function arguments is also defined as left to right but currently isn't supported by dmd.

Copy link

Choose a reason for hiding this comment

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

It's not about compiler semantics, It's bad coding style because you have to remember the rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not about compiler semantics, It's bad coding style because you have to remember the rule.

Looks like your personal opinion as I have never heard this before.
But lets think about it. If behavior here is undefined, the compiler will obviously reject such code just like it rejects int a = b, b = 2;. This rule is same even in C++. And for the reader this code is having a single meaning so, if the reader didn't know about this rule, such code will just help him to learn it and be happy. So I see no reasons against this code.

Copy link
Member

Choose a reason for hiding this comment

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

@denis-sh: A large number of coding guidelines require/recommend to have only one declaration by line, so I'm somewhat surprised you have never heard of it before. I agree with Andrej here – don't write needlessly confusing code, especially if it can be trivially avoided.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 7, 2012

Fixed comments and style issues pointed out by @AndrejMitrovic.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 8, 2012

To @jmdavis (sorry for the delay):

As far as the type system is concerned, the reference is the class.

Wow! You want to say that someClassRef.sizeof will return class instance size or someClassRef = null will rewrite class instance content? We both now it's not. The reference to class is the reference to class, not a class instance. That's all.

What you're trying to do seems to be to have destroy operate not just on what was passed to it but also everything that it indirectly refers to. That's a huge change to how it functions. It's fundamentally different, and it makes it far more dangerous, especially when you consider classes which hold other classes. There's a huge difference between destroying an object and destroying everything that it refers to.

You misunderstood my change. My change is:

  1. Fix destroy to work with static arrays;
  2. Rename destroy for classes to finalizeClassInstance;
  3. Rename for other types to destruct;
  4. Add destruct for classes that works like destruct for pointers i.e. just clears the ref.

In the pull description I didn't want to go by refs and destroy everything. I just noted that is is inconsistent that it goes by ref in one particular case.

It seems to me that the problem is that you have a fundamental misunderstanding of destroy rather than there being anything actually wrong with it. destroy is essentially the same as calling the destructor on something and setting it to its init value. It's what happens with objects passed to out parameters. What you're trying to make it do is very different.

Looks like you just proved that you are confused how destroy works too as class instance is not finalized when passed as out (what destroy does) but instead class reference is cleared (what destroy does not do).


Sorry, but as you see, I think it is you who is incorrect here. So I want somebody third to point out who is incorrect.

@alexrp, @AndrejMitrovic, anybody else, tell me that I'm blaming good people and show me my mistakes, please.

@denis-sh
Copy link
Contributor Author

denis-sh commented Nov 8, 2012

And in fewer words about what I want to do is:

1 . I want to have a destruct that will do this:

T t = ...;
...
// Should be equivalent to `destruct(t);`:
destruct(*cast(T[1]*) t);

As we can always assume that T is essentially the same as T[1].

2 . I want to have a function to finalize class instance.

@jmdavis
Copy link
Member

jmdavis commented Nov 8, 2012

I corrected myself with regards to out. out's behavior is the same as destroy save in the case of classes, because there's no way to separate the class instance from its reference. So, while you can do destroy(ptr) and destroy(*ptr), class references only have destroy(obj).


static if(is(T == struct) || is(T U : U[n], size_t n))
{
// Note: old `destroy` version just fills shared static arrays with its `init`,
Copy link
Member

Choose a reason for hiding this comment

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

Makes no sense to kick the old horse. Let the bones rest and the history stay in commits of the master branch.

@andralex
Copy link
Member

andralex commented Dec 9, 2012

If this doesn't do what it takes, then there's a problem elsewhere:

void destroy(T : U[n], U, size_t n)(ref T obj)
{
    obj = T.init;
}

People will routinely assign one array to another, and if that doesn't work properly then we have a bigger problem on our hands.

@jmdavis
Copy link
Member

jmdavis commented Dec 9, 2012

It would definitely be bad to call the finalizers of all of the elements of a static array, because they could be referred to elsewhere. For instance, if you had Object[10], there's no reason to finalize any of the Objects. Other stuff could refer to them. If they're structs, their destructors do need to to be called, but that should happen with just obj = T.init. If the caller wants to explicitly finalize each of the members of the static array, then they'll need to iterate over it and destroy each one in turn. However, what makes me curious is the implementation for destroying a struct:

void destroy(T)(ref T obj) if (is(T == struct))
{
    typeid(T).destroy( &obj );
    auto buf = (cast(ubyte*) &obj)[0 .. T.sizeof];
    auto init = cast(ubyte[])typeid(T).init();
    if(init.ptr is null) // null ptr means initialize to 0s
        buf[] = 0;
    else
        buf[] = init[];
}

Why doesn't it just do object = T.init? It should be in exactly the same boat as the static array. Either the struct overload is taking something into account that the static array overload is not, or the struct overload is doing stuff that it shouldn't be.

The only thing that I can think of would be that someone was worried about an overloaded opAssign. Since if opAssign were overloaded, the destructor for the object would not be called when it's assigned its init value (unless T.init is treated specially somehow). Its members would presumably be replaced (and their destructors called when reassigned - if they didn't overload opAssign), but the top-level struct's destructor wouldn't be called. If that's actually a problem, then obj = T.init is not sufficient for static arrays, and they'll need to make sure that each element is destroyed if they're structs (but finalizing class objects would definitely be the wrong behavior - it's just the stack values which need to actually be destroyed - it's the static array that's being destroyed after all, not it's elements, and that only means destroying the elements if they're on the stack with it instead of being reference types).

So, there may very well be a bug with how static arrays are handled. But Denis has never explained what he thought was buggy about destroying static arrays. Rather, he's complained about how destroy is broken in its very concept, and pretty much no one here as agreed with him on that point.

@DmitryOlshansky
Copy link
Member

It would definitely be bad to call the finalizers of all of the elements of a static array, because they could be referred to elsewhere. For instance, if you had Object[10], there's no reason to finalize any of the Objects.

So do the structs as there are possible pointers to them. It's expected that caller has knowledge showing it's safe to destroy/finalize something explicitly. It's always unsafe on its own.

The thought about finalizing each instance of array is along the lines of:
destroying a static array <---> destroying each element
Then each class instance has to be finalized since destory finalizes a single value when called directly.

If they're structs, their destructors do need to to be called, but that should happen with just obj = T.init.

I expect that assigning static array is equivivalent to an element-wise assignment and then overloaded opAssign can hijack the supposed destruction.

@jmdavis
Copy link
Member

jmdavis commented Dec 9, 2012

@blackwhale

The thought about finalizing each instance of array is along the lines of:
destroying a static array <---> destroying each element
Then each class instance has to be finalized since destory finalizes a single value when called directly.

That's fundamentally different from what obj = T.init does even ignoring issues with overloaded opAssign. But aside from the issues with overloaded opAssign, classes are the only case where destroy is not supposed to be equivalent to obj = T.init, so I don't know.

Ideally, you'd have to dereference the class to destroy it (as happens with pointers to structs), but because references aren't separated from what they point to in the type system (they're exactly the same type - there is no concept of the type being pointed to), that doesn't work, and destroy is forced to destroy the object referred to. So, classes function slightly differently from everything else, but the rest is completely consistent.

At this point, I think that it's clear that

  1. `destroy should actually destroy classes - i.e. call their finalizers. That's the main reason that it exists in the first place.
  2. For all other types, (ignoring issues with opAssign) it should do the equivalent of obj = T.init.
  3. For structs and static arrays, we may need to do something other than obj = T.init in order to really do obj = T.init like we mean to, because of overloaded opAssigns.

The question then that you bring up is destroying the elements in a static array. You're proposing that each element be destroyed individually, but that would be inconsistent with both obj = T.init, and it's inconsistent with how structs are handled. Their member variables are not destroyed. Rather, they're simply set to their init values. So, if this struct is destroyed

struct S
{
    Object a;
    Object b;
}

then the objects referred to by a and b continue to happily exist on the heap without being destroyed. I see no reason to treat static arrays any differently. If someone wants to actually destroy each of the elements, they can iterate over them, just like they can individually destroy the members of a struct. The only case where classes are currently destroyed is when you call destroy on them directly, and I think that it makes sense to continue with that. The entire design of destroy at this point is to destroy the top layer, not the layers underneath.

It may make sense to look into adding a function which recursively destroys an object rather than just its outer layer, but I don't think it makes sense to make destroy try and do any kind of recursive destruction. That's fundamentally different from what it does now.

@monarchdodra
Copy link
Contributor

I don't think it makes sense to make destroy try and do any kind of recursive destruction.

I completely disagree. I see 0 point in destroy not recursively destroying its internal stuff by default. Can you name a single usecase where it would make sense for destroy to call the current struct's destructor (or array's: nothing), but stop there?

When I write s.destroy;, then I'd expect s to be completly destroyed, innards and all, and not just some "outer layer". That'd be a complete violation of encapsulation. Also, when I call someStaticArray.destroy(), then what else do you think I'm asking for if it's not element-wise destruction? That I'm asking for a function that does nothing?

I also think that finalizeClassInstance is a very good idea, and that destroy should not destroy classes. For all intents and purposes, except for operator== and operator(<>=), a class handles like a pointer. Why make destroy special case for them?

If someone wants to actually destroy each of the elements, they can iterate over them

But do you realize how involved that is? How bug prone it is? No-one should have to write that himself. Not to mention, you'd have to iterate, checking that the sub elements are not classes, and only selectively destroy certain stuff...

Even if the library provides a special destroyRecursively, why would anyone even think they'd need that when they already have destroy?

That's fundamentally different from what it does now.

Then I'd say it's ether fundamentally broken, or fundamentally useless :/

Long story short, I fully support the intent of this pull (but I can't judge on the details).


Related: I just realized and am super-surprised that __postlit is also non-recursive. WTF? Why would anyone want to only postblit only the outer layer? I realize the answer is probably because it is the compiler that is responsible for doing the recursivity, but in that case, we need to have a library function called postblit that does what a coder would actually expect out of calling __postblit...

@jmdavis
Copy link
Member

jmdavis commented Dec 9, 2012

@monarchdodra

Pretty much the only reason that destroy exists in the first place is so that classes can be destroyed. In fact, when TDPL talks about it (under the name clear), the only thing that it talks about is clearing class objects, never structs or arrays or anything else. The entire purpose of destroy is to destroy an object without clearing its memory. That does not involve any kind of recursive destruction. All value types inside of a class would be destroyed, because they're value types, but nothing else is destroyed - nothing else has its finalizer called. destroy is basically trying to do what the GC would do with classes except that it's not freeing the memory, and it doesn't leave the class in a garbage state (I believe that it present, it puts it to its init state and zeroes out the vtbl in order to catch anything which uses the object after it's destroyed. It used to just put it in its init state, and it might just zero out the vtbl now, but that would cause problems with non-virtual functions if it did).

The fact that destroy works with anything other than classes is just gravy. Its whole purpose is to destroy classes on the heap without deallocating their memory. And recursive destruction doesn't enter into that. destroy is specifically destroying the class object so that anything that the finalizer does is done and so that any reference types that the object refers to are then free to be collected by the GC if nothing else refers to them. Actually trying to finalize them too could cause all kinds of problems.

Image the havoc that you'll cause if your object (which is arbitrarily complex) suddenly finalizes all of its reference members when being destroyed. They could be pointed to by who-knows-what, and there's a good chance that you don't have any way of knowing what. It's also fundamentally different from what the GC does when finalizing objects, which completely goes against the goal of destroy.

Now, given that destroy works with types other than classes, it needs to be made to do so sanely. The idea with classes is that they're finalized, put in their init state, and then have their vtbl zeroed out for good measure to help ensure that things will go boom if you're careless enough to use the destroyed object. The closest equivalent for the other types would therefore be to destroy them and then put them into their init states. In most cases, that means simply putting them in their init state, because they have no destructor. For structs however, that means calling the destructor before putting them in their init state. And normally, simply putting them in their init state would cause the destructor to be called first, making even calling the destructor unnecessary, but overloaded opAssign gets in the way of that.

The GC never recursively destroys any reference types, and neither should destroy. Recursively destroying value types makes sense, because they live in the object that's being destroyed, but reference types are merely referred to by the type that they're in. They'd have to be explicitly owned by the object that they're in for destroying them to make sense, but they're not. Something else owns them (generally the GC), and it should be its job to take care of them. Destroying an object is unsafe enough in the first place, but it's insanely unsafe to even consider having a class or struct finalize its reference members. And destroy should not be behaving fundamentally differently from how the GC behaves, which such recursive destruction would do.

@denis-sh
Copy link
Contributor Author

To @monarchdodra:
Sorry, but I see no need in destroyRecursively. You probably are a bit confused. I want (and, I hope, you want it too) to have a function that equals to go out of scope which is needed for all cases of manual object lifetime implementation (it is in rejected pull dlang/phobos#929).

Related: I just realized and am super-surprised that __postlit is also non-recursive. WTF? Why would anyone want to only postblit only the outer layer? I realize the answer is probably because it is the compiler that is responsible for doing the recursivity, but in that case, we need to have a library function called postblit that does what a coder would actually expect out of calling __postblit...

Everything you need is in pull dlang/phobos#928!


To @jmdavis:

Ideally, you'd have to dereference the class to destroy it (as happens with pointers to structs), but because references aren't separated from what they point to in the type system (they're exactly the same type - there is no concept of the type being pointed to), that doesn't work, and destroy is forced to destroy the object referred to.

I can only repeat: classes and class references has the same type and that's all. In any other view it differ. Examples: only class reference is passed by value, .sizeof and other properties are all about a class reference only and for class instance there are special stuff like __traits(classInstanceSize, ...).

It may make sense to look into adding a function which recursively destroys an object rather than just its outer layer, but I don't think it makes sense to make destroy try and do any kind of recursive destruction. That's fundamentally different from what it does now.

As I answered to @monarchdodra, is see no reason for it but I see a strong reason to have a function that equals to go out of scope.

The fact that destroy works with anything other than classes is just gravy. Its whole purpose is to destroy classes on the heap without deallocating their memory. And recursive destruction doesn't enter into that.

The fact that destroy works with anything other than classes is dangerous as it is confusing and error-prone. I will be very happy once it will work only for classes. I also would like it to be called finalizeClassInstance.

@monarchdodra
Copy link
Contributor

The fact that destroy works with anything other than classes is just gravy. Its whole purpose is to destroy classes on the heap without deallocating their memory. And recursive destruction doesn't enter into that. destroy is specifically destroying the class object so that anything that the finalizer does is done and so that any reference types that the object refers to are then free to be collected by the GC if nothing else refers to them. Actually trying to finalize them too could cause all kinds of problems.

Image the havoc that you'll cause if your object (which is arbitrarily complex) suddenly finalizes all of its reference members when being destroyed. They could be pointed to by who-knows-what, and there's a good chance that you don't have any way of knowing what. It's also fundamentally different from what the GC does when finalizing objects, which completely goes against the goal of destroy.

I think there may be some confusion about what you and I call recursive. In particular, I just realized that typeid(T).destroy() already does correctly (and recursively) destroy the passed in object.

EDIT: Yes, as @denis-sh said, there was some confusion. I never even imagined to fetch out reference types and destroy those.

You say:

The question then that you bring up is destroying the elements in a static array. You're proposing that each element be destroyed individually, but that would be inconsistent with both obj = T.init, and it's inconsistent with how structs are handled. Their member variables are not destroyed. Rather, they're simply set to their init values. So, if this struct is destroyed.

This is what is confusing me, because it isn't true:

import std.stdio;

struct A
{
    ~this(){"bla".writeln();}
}
struct B
{
    this(this){};
    A a;
}

void main()
{
    B b;
    B[2] bb;
    C[2] cc = [new C(), new C()];
    typeid(B).destroy(&b);
    writeln("1");
    typeid(B[2]).destroy(&bb);
    writeln("2");
    typeid(C[2]).destroy(&cc);
    writeln("3");
}
bla
1
bla
bla
2
3
bla
bla
bla
C
C

As you can see, destroying b triggers a's destruction. destroying bb triggers the destruction of each (value type) element.


So while I understand what is going on better now, and don't have any deep issues anymore, I still have to agree with @denis-sh that putting both structs and classes in the same bag in regards to destroy is strange at best.

Proof that it's weird? class instances are not destroyed when called from some encapsulating object (structs containing a class, static array containing a class), yet if you call destroy on a class, it finalizes it...? I'd have expected it just sets the reference to 0.


Related: I found typeid(B).postblit(), so ignore my above comment regarding __postblit.

@denis-sh
Copy link
Contributor Author

To @monarchdodra:

Related: I found typeid(B).postblit(), so ignore my above comment regarding __postblit.

NO! Don't use it in your code! typeid is very complex and has lots of hidden dangers! It will always compile, work in some cases and fail in other!

@denis-sh
Copy link
Contributor Author

To @alexrp and @blackwhale:
We can not use obj = T.init; because:

  • elaborate assign
  • const/immutable fields
  • compiler bugs (yes, this fill gone some day)

@denis-sh
Copy link
Contributor Author

To @andralex:

If this doesn't do what it takes, then there's a problem elsewhere:
...
People will routinely assign one array to another, and if that doesn't work properly then we have a bigger problem on our hands.

Destroying isn't an assignment. It must work where assignment will not. Se my previous post.

@denis-sh
Copy link
Contributor Author

To @blackwhale:

@denis-sh - could you please specify unittests that fail today for static arrays and structs with one member? Thanks.

BTW I think that was a very sensible request just paste that as code and let people "discover" problems.

Posted move is such example.

As for "Andrei expectations: ... " I would first and futhermost suggest you not to try to put words in somebody else mouth. It doesn't help this proposal at all to go into personal battles and I see the a lot of provacation in this thread.

Andrei is here and always can say that his expectations differ. I will then change my post and that's all so I see no problem here.

As for the proposal and a sample of move represented. It entierly depends on what destroy and setInitialState do.

I described setInitialState in comment, what is unclear? As for destroy I assumed that is a current destroy but without bugs (working as it is currently supposed to work).

@denis-sh
Copy link
Contributor Author

To @jmdavis, @blackwhale, @monarchdodra:
As you are already here (thanks a lot, really!) can you answer to my question above?

That question post has nothing to do with insulting of Andrei and you can ignore "Andrei expectations" (but I really think that it is his opinion).

So, please, give me the answers!

@andralex
Copy link
Member

A part of the discussion above boils down to the question: what does it mean to call destroy(obj) where obj is of various kinds: primitive type, struct instance, class instance, static array, dynamic array. What I think should happen:

  • If obj is a primitive type, do nothing.
  • If obj is a struct object and the struct has a non-trivial destructor (either explicitly defined or implicit by means of embedding fields with non-trivial destructors), call the destructor and then obliterate obj with its init bits. Otherwise, do nothing.
  • If obj refers to a class instance and the class has a non-trivial destructor (either explicitly defined or implicit by means of embedding fields with non-trivial destructors), call the destructor and then obliterate the class instance with its init bits.
  • If obj is a static array, treat it as a struct with fields of the element type.
  • If obj is a dynamic array, treat it as a struct with fields of the element type. (This is currently not implemented.)

I understand there are other possible designs and design variations. The rationale for this design is that it has simple semantics and consistently handles static arrays similarly to structs, and dynamic arrays similarly to classes.

@denis-sh could you please rework this pull request into implementing this semantics? Let's get this done and make progress. Thanks.

@denis-sh
Copy link
Contributor Author

If obj is a dynamic array, treat it as a struct with fields of the element type.

Do you mean to destory every array element? If so, destroy will became incredible complicated and inconsistent IMHO.

Anyway, your proposed semantics doesn't look simple at all. This semantics is what I think error prone, dangerous and inconstant.

@andralex
Copy link
Member

If obj is a dynamic array, treat it as a struct with fields of the element type.

Do you mean to destory every array element? If so, destroy will became incredible complicated and inconsistent IMHO.

The purpose of destroy is to reclaim non-memory resources without compromising memory safety. So it makes perfect sense to destroy the contents of a dynamic array whilst keeping its allocated memory.

Anyway, your proposed semantics doesn't look simple at all. This semantics is what I think error prone, dangerous and inconstant.

Understood. This pull is not acceptable in its current form, and seeing as this has become a time sink without much chance to make progress, I will close it now. Thanks for your work, and apologies for the trouble. Feel free to reopen if new arguments come on the table.

@andralex andralex closed this Dec 10, 2012
@denis-sh
Copy link
Contributor Author

The purpose of destroy is to reclaim non-memory resources without compromising memory safety. So it makes perfect sense to destroy the contents of a dynamic array whilst keeping its allocated memory.

I'm not against this definition of destroy. The problem is that it accepts any type and do something with it which very a lot depending on a type. This is a situation when function overloading should not be used. Why not to create finalizeClassInstance and finalizeDynamicArray functions?

@andralex
Copy link
Member

I'm not against this definition of destroy. The problem is that it accepts any type and do something with it which very a lot depending on a type. This is a situation when function overloading should not be used. Why not to create finalizeClassInstance and finalizeDynamicArray functions?

Forms follows function. To destroy things one calls destroy.

@denis-sh
Copy link
Contributor Author

Forms follows function. To destroy things one calls destroy.

So you are against that it is obviousness how destroy behaves for different argument types?

@denis-sh
Copy link
Contributor Author

Everybody like function overloading but there are some situations it can't be used.

This is such situation because:

  1. Too easy to make a mistake in a templated code which is hard to find/debug.
  2. No need for function like current destroy.

What in this two statements not obvious?

@denis-sh
Copy link
Contributor Author

By the way, I filled Issue 9137 - A function that equals to "out of scope" action for manual lifetime management to allow you all to destroy me there and mark it as INVALID once and forever.

@denis-sh
Copy link
Contributor Author

And I also filled Issue 9139 - destroy is dangerous and inconsistent to allow you mark it as INVALID too.

@andralex
Copy link
Member

Thanks. My purpose is not to mark valid concerns as invalid, so the sarcasm is unwarranted there. On the other hand, please understand I can't accept a design I disagree with just to be nice. You have made an argument. It has been well understood and debated at length. Past this point rehashing the issue without fresh perspectives becomes a net loss of time for those involved. There is something to be said about picking one's fights and doing the best possible with one's free time. Above that, a collegial attitude is always appreciated and respected. Thanks.

@denis-sh
Copy link
Contributor Author

@andralex:

@denis-sh could you please rework this pull request into implementing this semantics? Let's get this done and make progress. Thanks.

Created #362 for you. Its first commit equals the first commit from this pull and its second commit apply your change.

@ghost
Copy link

ghost commented May 21, 2014

I haven't seen this mentioned here: is it ok that currently .destroy(sp); where sp is a struct pointer is a no-op? You have to call .destroy(*sp); instead (this does work even if the struct has a disabled postblit).

It's a bit error-prone to have no-ops like this. @andralex thoughts?

@denis-sh
Copy link
Contributor Author

It's a bit error-prone to have no-ops like this.

destroy behavior is as planned here. The problem is in a counterintuitive design.

@ghost
Copy link

ghost commented May 21, 2014

destroy behavior is as planned here.

Then it should be documented, I don't see any mention of pointers in the code.

@schveiguy
Copy link
Member

is it ok that currently .destroy(sp); where sp is a struct pointer is a no-op?

I think it's implemented as designed, but I think there should be an appropriate "destroyRef" function, which destroys classes, and destroys the target of pointers, depending on the arg. Currently, it is possible, and quite simple, to create a type that keeps a reference to a type, and not care whether it is a class or a struct pointer, since a.b works on both. But destroy will not work correctly on the struct pointer.

Consider that delete would work equally on both, and destroy is supposed to be the answer to deprecation of delete

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants