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

Fix destroy #362

Closed
wants to merge 2 commits into from
Closed

Fix destroy #362

wants to merge 2 commits into from

Conversation

denis-sh
Copy link
Contributor

This pull fixes incorrect destroy behavior and makes destroy reclaim non-memory resources also for dynamic arrays as Andrei requested.

* breaking change: reject shared static arrays (silently worked incorrect previously)
* breaking change: do not reject nested static arrays (rejected previously)
* also add unittests
@denis-sh
Copy link
Contributor Author

@andralex, is it what you were asking for?

else
buf[] = init[];
}
static assert(!is(T == const), "`destroy` doesn't work for `const` types");
Copy link
Member

Choose a reason for hiding this comment

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

This is overly conservative. We should be able to e.g. destroy a const int or a pointer or an enum value etc. because that's a no-op. So we should only disallow overwriting const/immutable data, but other than that no problem. (There is this weird corner case of destroying const objects without indirections and without mutable indirections... we should discuss that later.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not finally fix current destroy implementation (like I did with emplace) as I hate it. You can accept or reject these fixes, I will not add more fixes here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andralex,
this your comment shows that you think destroy can be used in templated code. And I'm sure it is incorrect as destroy behaves differently for different types and you always have to wrap it in static if or you will get compilable program that will fail at runtime once because you didn't predict some case of destroy behavior.

I was wrong. I don't understand what is going on in your brain at all. Please, do not be hurt, but when I'm reading your posts about destroy I think that you do not see this problem at all and you even don't want to try to see this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not add more fixes here.

More than that, you const int proposal isn't a fix, it only increases destroy danger. The more types destroy accept the higher probability one will write incorrect templated code with destroy and will spend tons of his time for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andralex,
sorry again as this time it really sounds offensive, do you understand that you can't show me (and, probably, even imagine) any example of templated function using destroy for arbitrary types and still trying to make it work for more and more types?

@andralex
Copy link
Member

It seems we're not making progress in this matter. I will close this and get to improving destroy later. @denis-sh feel free to leave this one as it is and focus on other areas. Thanks.

@andralex andralex closed this Dec 10, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants