implicit-clone icon indicating copy to clipboard operation
implicit-clone copied to clipboard

Add alteration methods to IArray

Open ColonelThirtyTwo opened this issue 2 years ago • 11 comments

Includes IArray::make_mut, which works similar to Rc::make_mut, as well as insertion and removal methods, all of which copy-on-write the array.

ColonelThirtyTwo avatar Oct 22 '23 03:10 ColonelThirtyTwo

Have you participated in #17 discussion?

I believe your solution is the same as using .to_vec(), except some cases where Rc does not have any weak refs and have only one current strong ref. If #17 is considered when making this, then I believe it should close #17 too.

kirillsemyonkin avatar Oct 22 '23 07:10 kirillsemyonkin

I imagine this is being blocked by #35.

kirillsemyonkin avatar Oct 22 '23 08:10 kirillsemyonkin

Have you participated in #17 discussion?

I believe your solution is the same as using .to_vec(), except some cases where Rc does not have any weak refs and have only one current strong ref. If #17 is considered when making this, then I believe it should close #17 too.

I did not. The im/im-rc crate already implements a tree-based CoW Vec that does structural sharing like #17 seems to talk about, so IMO the authors should use that if that's the semantics they want. I'm not a huge fan of it because of worse random access times and the fact that the array is no longer a slice.

ColonelThirtyTwo avatar Oct 22 '23 17:10 ColonelThirtyTwo

I was not talking about HAMT, I was talking about the rest...

kirillsemyonkin avatar Oct 22 '23 18:10 kirillsemyonkin

I was not talking about HAMT, I was talking about the rest...

Yes actually I'm not sure this API proposed in this PR here is a good idea. Maybe the "to_vec()" is more than enough and more rust-y. It discussed more about it on #17 with my reasoning.

I'm not strongly against either so I will go with the flow.

I did not. The im/im-rc crate already implements a tree-based CoW Vec that does structural sharing like #17 seems to talk about, so IMO the authors should use that if that's the semantics they want. I'm not a huge fan of it because of worse random access times and the fact that the array is no longer a slice.

It might be worth adding an optional dependency on im/im-rc that will allow the user of implicit-clone to get the trait ImplicitClone on im/im-rc's types? This idea is really out of scope with this PR though.

cecton avatar Oct 23 '23 13:10 cecton

I didn't even notice all of the methods added by this PR, only paying attention to those in the PR's description, oops...

I personally am not against make_mut and get_mut (where is it?), but I'm still not sold on everything else, as it encourages cloning the whole array on every action (@cecton I'm still shocked IArray's iter() clones every item, even though reads intuitively should happen way more often than writes, and it would be nice to not need them cloned in all of their entirety all of the time. I guess the rationale is that all items are ImplicitClone, and we actually do not allocate the array memory space for containing these clones).

To me, the non-resizable [T] which is given by both of them sounds like a good idea. These two functions would be good for the user to be able to quickly mutate the elements themselves after making an IArray before sending it out to the world.

kirillsemyonkin avatar Oct 23 '23 17:10 kirillsemyonkin

@cecton I'm still shocked IArray's iter() clones every item, even though reads intuitively should happen way more often than writes, and it would be nice to not need them cloned in all of their entirety all of the time. I guess the rationale is that all items are ImplicitClone, and we actually do not allocate the array memory space for containing these clones).

haahh you're shocked for nothing :P really, what do you think happens in other programing languages anyway? And yes the rational was that each element of the IArray must implement ImplicitClone so they are supposed to be cheap to clone. Now it's not like I particularly mind about this API at all. It's not very rust-y but it allows writing fp-style code easily

wanna join 2 collections? array1.iter().chain(array2.iter()).collect(), no need to bother with the references. You want to iterate on a list of items like you would in JS, you can do it easily because you don't have the additional cognitive load of the borrow checker of Rust.

To me, the non-resizable [T] which is given by both of them sounds like a good idea. These two functions would be good for the user to be able to quickly mutate the elements themselves after making an IArray before sending it out to the world.

That's what the implicit cloning of IArray is actually trying to bring. Since the elements are cheap-to-clone, we are allowed to write things almost like a dynamic language. But again I don't have a strong opinion on this, it was a 0.x version.

(IMap also has this kind of API in mind btw brining dynamic programming to rust, truly going against the norm and see what does it do. Maybe it is OP, maybe it's terrible lol, maybe both)

cecton avatar Oct 24 '23 08:10 cecton

You want to iterate on a list of items like you would in JS, you can do it easily because you don't have the additional cognitive load of the borrow checker of Rust.

I don't agree. The Rust-ism is that .iter() on collections returns references. Having to work with IArray that breaks this convention would increase my cognitive load, because now I have to remember the special case of IArray returning clones and not references.

EDIT: In addition, you do not want to be frivolously cloning Arcs. The atomic operations they do have more overhead than the non-atomic Rc's and can't be optimized out as easily.

wanna join 2 collections? array1.iter().chain(array2.iter()).collect(), no need to bother with the references.

array1.iter().chain(array2.iter()).cloned().collect() isn't that much bigger.

EDIT2: You also can't borrow from the iterator if it clones:

let arr: Vec<Rc<ReallyBigType>> = ...
// can't do this with IArray without cloning every node.string
let field = arr.iter().find(|node| &node.string).collect::<String>();

ColonelThirtyTwo avatar Oct 24 '23 18:10 ColonelThirtyTwo

I think we're going to need this in Yew for the ChildrenRenderer

Post code comparisons here? Hopefully you are not trying to add individual items in a loop?

kirillsemyonkin avatar Nov 01 '23 16:11 kirillsemyonkin

I was going to make the PR now ^_^

cecton avatar Nov 01 '23 16:11 cecton

Wait no, not for this. The mutable API would be useful to use IArray in VList instead of Vec. Though I'm not entirely sure how efficient this is... maybe it's terrible

cecton avatar Nov 01 '23 17:11 cecton