Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove destructor from IRFunction #214

Merged
merged 1 commit into from
Mar 6, 2018
Merged

Remove destructor from IRFunction #214

merged 1 commit into from
Mar 6, 2018

Conversation

JinShil
Copy link
Contributor

@JinShil JinShil commented Mar 6, 2018

@andralex is proposing a change to D's destroy implementation at dlang/druntime#2126

The new semantics re-initialize an instance if it has a user-defined destructor. This breaks Higgs as shown in the output of D's Jenkins CI (https://ci.dlang.io/blue/organizations/jenkins/dlang-org%2Fdruntime/detail/PR-2126/1/pipeline/235)

Since IRFunction's destructor does nothing, I think it is safe to delete, regardless of whether @andralex's proposed implementation of destroy is accepted or not. And, D will be able to continue to test compatibility with Higgs in its CI.

cc @maximecb @sbstp

@maximecb
Copy link
Contributor

maximecb commented Mar 6, 2018

Destructors reinitialize instances? That sounds like a terrible design choice.

I will approve the PR since the destructor is not needed. Guess I should have stuck more closely to the YAGNI principle ;)

@maximecb maximecb merged commit dbe1fb9 into higgsjs:master Mar 6, 2018
@andralex
Copy link

andralex commented Mar 6, 2018

Destructors reinitialize instances? That sounds like a terrible design choice.

Not destructors, but the destroy function, which has the awkward position of calling the destructor yet leaving the object in a usable state. That is a special situation that needs careful handing.

We'd appreciate alternatives, and this is the right time to propose them (with dlang/druntime#2126). Somewhat ironically relative to the remark, the proposed semantics do less/minimal reinitialization post destruction. It's limited to cases like this:

struct A {
    ~this() { ... }
}
void fun() {
    A a;
    ...
    destroy(a);
}

Following the call to destroy, the destructor of A will still be called, resulting in undefined behavior (destructors are only called on valid objects and are not required to support multiple calls).

Another (probably more common) case is that some code calls destroy(a) then attempts to reinitialize a. If the attempt throws an exception, again we're facing a double destruction.

To improve on situations like this, destroy puts the object in the .init state (no resources, but destroyable state) only for types that define a destructor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants