Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Various type and trans cleanups. #11252

Closed
wants to merge 3 commits into from

6 participants

@eddyb
Collaborator

No description provided.

@brson
Owner

Need someone who knows trans to review: @nikomatsakis @pcwalton ?

@nikomatsakis
Owner

I can review, but not until next week.

@eddyb
Collaborator

The last commit requires cleanup improvements (which @nikomatsakis said was working on), right now this example crashes with a double free:

// Because ~[u8], it's represented as a pointer to len, capacity and data,
// so it would be passed ByValue (datum-wise).
fn foo(x: ~[u8]) -> ~str {
    unsafe {
        // This call would move out of x, so the original cleanup
        // has to be revoked. but this isn't in the same scope as
        // the original, so only ByRef(ZeroMem) can be revoked
        // and that produces bloated code (which I wanted to avoid).
        ::std::unstable::intrinsics::transmute(x)
    }
}
fn main() {
    foo(~[]);
}

EDIT: I've delayed fixing that issue, see #11445 - the code for determining whether an alloca is (not) required is still there, but it's disabled.

@eddyb
Collaborator

From my own tests, it looks like librustc takes 20s (about 3%) less to compile and link (I assume most of that is caused by the removal of free_glue).

@pcwalton pcwalton commented on the diff
src/librustc/middle/trans/base.rs
((73 lines not shown))
let _icx = push_ctxt("copy_args_to_allocas");
let mut bcx = bcx;
match fcx.llself.get() {
Some(slf) => {
- let self_val = if slf.is_copy
- && datum::appropriate_mode(bcx.ccx(), slf.t).is_by_value() {
- let tmp = BitCast(bcx, slf.v, type_of(bcx.ccx(), slf.t));
- let alloc = alloc_ty(bcx, slf.t, "__self");
- Store(bcx, tmp, alloc);
- alloc
+ let needs_indirection = if slf.mode.is_by_value() {
+ // FIXME(eddyb) #11445 Always needs indirection because of cleanup.
@pcwalton Owner

I'd prefer to just have commented out code or something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/librustc/middle/trans/meth.rs
((6 lines not shown))
let mut temp_cleanups = ~[];
- let Result {bcx, val} = trans_self_arg(bcx, this, &mut temp_cleanups, mentry);
+ let Result {bcx, val} = trans_arg_expr(bcx, self_ty, this,
+ &mut temp_cleanups,
+ DontAutorefArg);
+ // HACK should not need the pointer cast (add self in trans_fn_ref).
@pcwalton Owner

What does "add self in trans_fn_ref" mean?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pcwalton
Owner

This looks great modulo nits, thanks so much!

@huonw
Owner

r=pcwalton

Collaborator

saw approval from pcwalton
at eddyb@08ac616

Collaborator

merging eddyb/rust/ty-cleanup = 08ac616 into auto

Collaborator

eddyb/rust/ty-cleanup = 08ac616 merged ok, testing candidate = 4bdda35

Collaborator

fast-forwarding master to auto = 4bdda35

@bors bors closed this
@eddyb eddyb deleted the eddyb:ty-cleanup branch
@nikomatsakis

For what it's worth, I read these changes over on the plane and was coming here to give them r+ as well, but I see that @huonw beat me to the punch. Thanks @eddyb. Now I just have to rebase my branch over these changes -- actually that doesn't look too bad.

@huonw
Owner

(It was actually @pcwalton who r+'d it originally; there were just some minor things with the testsuite on the first few runs.)

@eddyb
Collaborator

@nikomatsakis so I won't forget: if an argument doesn't require drop, it doesn't need indirection, the current override is stronger than it needs to be. I hope it will be possible to remove the indirection even for types with drop in the future, at least in simple cases like my example above (which is the same as #11445).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.