Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Overloaded operators can copy self when self is noncopyable. #2548

Closed
eholk opened this Issue · 9 comments

4 participants

@eholk
resource r<T> (_x: T) { io::println("Goodbye, World!") }

fn main() {
    let mut res = r(5);

    let mut v = [mut];
    v <- [mut res] + v;
}

This outputs Goodbye, World! twice, when it should only do it once.

@eholk

This small variant has the correct behavior.

resource r<T> (_x: T) { io::println("Goodbye, World!") }

fn main() {
    let mut res = r(5);

    let mut v = [mut];
    v <- [mut res];
}
@eholk

This happens because trans does not know to move in tvec::trans_add. I'm attempting to fix this by moving vector addition to libcore.

@eholk eholk was assigned
@eholk

I just landed the "move vector-append out of trans" code, so this should be mostly fixed. Unfortunately, there's a bug in checking self types that means it's still not too hard to copy an uncopyable.

@hatahet

Are there 2 instances being created?

class r {
  let mut x: int;
  new() {
    self.x = 1;
  }
  drop {
    self.x += 1;
    io::println("self.x: " + int::str(self.x));
  }
}

fn main() {
  let mut res = r();

  let mut _v = ~[mut];
  _v <- ~[mut res] + _v;
}

Output:

self.x: 2
self.x: 2
@eholk

Yeah, that's the problem with this issue. Classes with a destructor are supposed to be non-copyable, but this shows how to trick the type system into copying one for you.

@nikomatsakis
Owner

I don't understand, what leads to the copy of self? @eholk can you update the title and maybe add a comment on what precisely is still wrong?

@eholk

@nikomatsakis I'll add an updated test case on this. After thinking about it for a bit, I think this might be a dup of #2587.

@eholk eholk referenced this issue from a commit
@eholk eholk Adding a test case for #2548 b3933b8
@eholk

Hmm, that test case should actually be a compile-fail test (unless + took ownership of self), since + is defined on vectors of copyable things.

@catamorphism

Should be fixed as of 46990ad

@jayanderson jayanderson referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
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.