Skip to content

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Apr 27, 2016

The only existing test we have relies on the simple in-place initialization behavior in old trans, e.g.:

    // Runs Foo's Drop impl 3 times.
    let _x = [x, Foo, Foo];
    // Runs Foo's Drop impl 4 times.
    let x = Foo;
    let _x = [x, Foo, Foo];

With MIR, there are more temporaries, and the test can't work without extra move tracking.

This is a [breaking-change], but #[unsafe_no_drop_flag] is unstable, and only works for ZSTs in very simple cases, so I doubt there are real uses in the wild. Fixes #33237.

@rust-highfive
Copy link
Contributor

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@pnkfelix
Copy link
Member

pnkfelix commented Apr 27, 2016

I would think that to truly resolve #33237, we should also have a run-pass test that explicitly shows how one would write a similar test (on a non-zero-sized type) that properly consults the drop-flag state to decide whether to run the destructor or not.

(That, or point to an existing unit test that is similarly demonstrating how to write such code.)


Update (hopefully a clarification): the point I am trying to make here is that the alternative code I wrote in #33237 is an example of behavior that we probably do not want to consider specified/stable, and it is on a type that is explicitly not zero sized.

I now understand why @eddyb is pointing out that part of the story here is to disallow #[unsafe_no_drop_flag] on ZST's (namely, because if you have a ZST and no drop flag attached, there there is quite simply no way you can hope to properly implement Drop for such a type, and thus @eddyb is flagging such types ahead of time).

@pnkfelix
Copy link
Member

(I want to discuss this with either T-compiler or T-lang before merging.)

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team I-nominated labels Apr 27, 2016
@eddyb eddyb force-pushed the no-zst-drop branch 2 times, most recently from 5698f9e to 1b80da0 Compare April 27, 2016 14:52
@pnkfelix pnkfelix added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Apr 28, 2016
@pnkfelix
Copy link
Member

@eddyb (if we do decide to land a check like this, it should be downgraded to a warning first, following something like RFC rust-lang/rfcs#1589 )

@pnkfelix
Copy link
Member

@eddyb (but first lets see what a crater run says)

@eddyb
Copy link
Member Author

eddyb commented Apr 28, 2016

@pnkfelix Does that apply to changes in unstable features (which this is)?

@pnkfelix
Copy link
Member

@eddyb yes I think this rule applies even for unstable features.

I can understand how that might sound odd, since someone who is using unstable features is already putting up with using the nightly channel (or a snapshot of nightly, or a locally built rustc).

But my thinking is that we want to make the runway for breaking changes a smooth incline, even when the breakage is solely applicable to unstable things.


@nikomatsakis may want to chime in further, or contradict me entirely for that matter. :) (rust-lang/rfcs#1589 doesn't draw a distinction AFAICT in terms of what kinds of features it is meant to apply to)

@DemiMarie
Copy link
Contributor

@eddyb I think part of the reason for this is that some crates require nightly Rust, as does Servo.

@eddyb
Copy link
Member Author

eddyb commented Apr 30, 2016

@drbo Sure, and they deal with fallout on virtually every single manual update of the Rust version they support (Servo in particular has a set of custom lints which use the unstable rustc API).

@bors
Copy link
Collaborator

bors commented May 11, 2016

☔ The latest upstream changes (presumably #33425) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member Author

eddyb commented May 13, 2016

Results of crater run: https://gist.github.com/83e72ed67d7424a37a1fa8e6937b245a.
Effectively 0 regressions (and a timeout).
However, I don't care enough, given that proper non-zeroing/filling dynamic drop could land in the following weeks.

@pnkfelix
Copy link
Member

@eddyb okay, well, r=me (just fix the tidy failures).

@@ -231,6 +231,11 @@ pub fn in_memory_type_of<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, t: Ty<'tcx>) ->
return llty;
}

// FIXME(eddyb) use the layout to actually compute the LLVM type.
Copy link
Member

Choose a reason for hiding this comment

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

(I said "r=me" but maybe this needs to actually be addressed first?)

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a long term goal that is blocked on removing old trans.

@eddyb
Copy link
Member Author

eddyb commented Jun 9, 2016

With non-zeroing drop here, there's no need for this.

@eddyb eddyb closed this Jun 9, 2016
@eddyb eddyb deleted the no-zst-drop branch June 9, 2016 20:49
@eddyb eddyb restored the no-zst-drop branch March 8, 2017 19:04
@eddyb eddyb deleted the no-zst-drop branch March 8, 2017 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-crater Status: Waiting on a crater run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants