capnproto / capnpc-rust

Cap'n Proto code generation for Rust
76 stars 26 forks source link

Changed member get accessors' self to readonly reference. #24

Closed giacomocariello closed 9 years ago

giacomocariello commented 9 years ago

Passing self to get accessors by reference instead of value helps avoiding move of instance after each call on Builder, which is not Copyable. I suppose passing by reference also helps with Reader's performance.

dwrensha commented 9 years ago

I don't think this will work, due to the reasons outlined at the end of this post.

giacomocariello commented 9 years ago

I must be missing something: I've replaced self with &self in init_ signatures of your example and it works correctly with the example in your blog post: lifetime of the object is correctly propagated.

I've posted an example here: https://github.com/giacomocariello/capnp_test which compiles and runs fine with rust stable.

Could you please show me what's wrong with my example?

dwrensha commented 9 years ago

The problem with this proposal is that it's not safe. It would allow multiple mutable references to the same data.

    let mut message3 = capnp::message::Builder::new_default();
    let foo = message3.init_root::<foo::Builder>();

    let b0 = foo.init_blob(10);
    let utf8_str = ::std::str::from_utf8(foo.get_blob().unwrap()).unwrap();

    // Now we can push non-utf8 data into utf8_str through b0, which aliases the same memory.
    b0[0] = 0xfa;
    b0[1] = 0xf2;
    b0[2] = 0xf0;
    println!("{}", utf8_str);
giacomocariello commented 9 years ago

I get your point. The post was really helpful. Thanks for your kind support.