Change result::map to consume the result #5840

Closed
wants to merge 4 commits into
from

Projects

None yet

6 participants

@erickt
Contributor
erickt commented Apr 11, 2013

This changes result::map to use moves instead of copies to transform one result into another. This makes the function usable when result is wrapping uncopyable values.

Also, it includes a minor cleanup of serialize.rs.

@erickt
Contributor
erickt commented Apr 11, 2013

Just rebased this onto HEAD, and added a minor fix to the deriving code that fixes the visibility to be inherited instead of public on the impls.

@thestinger
Contributor

Could you copy the naming conventions from option.rs for this? This would be map_consume, and map would use a reference.

@erickt
Contributor
erickt commented Apr 11, 2013

@thestinger: I'd rather it be the other way around. I expect (without proof) that most uses of map would be to consume options and results in a chain. I'll check if any of the users of option::map actually need to make a copy of the value.

@thestinger
Contributor

@erickt: that would be fine (map and map_ref), as long as it's consistent :)

@brson
Contributor
brson commented Apr 12, 2013

+1 to map and map_ref. Let's also change option/result get to behave like unwrap to mirror this behavior. Then add map_copy and get_copy.

@brson
Contributor
brson commented Apr 12, 2013

Let's do this for vectors too. map is consume

@thestinger
Contributor

@brson: I think the copy versions could be left out, x.clone().map or copying from the references is simpler.

@graydon
Contributor
graydon commented Apr 13, 2013

+1 but it needs rebasing again :(

@erickt
Contributor
erickt commented Apr 13, 2013

@brson: I was just about to submit an RFC to ask if we should change all the map fns to consume their input :) I would prefer to call these functions map_ref though instead of map_copy since types like vec won't actually need copying the inner value.

@erickt
Contributor
erickt commented Apr 13, 2013

Another option is to follow the style of vec::filter and vec::filtered, where filter is used when moving out of the container, and filtered creates a new container.

@erickt
Contributor
erickt commented Apr 14, 2013

@brson: I migrated vec::map et al to consume, but I'm hitting the cluster of bugs around #4759, where moving a value of a method and passing it to another function causes a double free. So I'm going to have to wait for that to be fixed before I can change all the map functions.

@brson
Contributor
brson commented Apr 16, 2013

fwiw I'm not all that fond of filter vs filtered, have a hard time remembering which does what.

@graydon
Contributor
graydon commented Apr 18, 2013

Yeah, filter / filtered is cute but not entirely obvious; filter alone (consuming argument) seems fine, users can clone when they need a copy.

@erickt
Contributor
erickt commented Apr 18, 2013

@graydon: I thought about getting rid of the filtered method, but there is a good reason to keep a function like that around. For example:

let xs = ~[1, 2, 3, 4, 5];
let ys = xs.filtered(|x| x == 1);

Only requires one copy, whereas with clone we copy every element before filtering down the list:

let xs = ~[1, 2, 3, 4, 5];
let ys = xs.clone().filter(|x| x == 1);

I'd like to keep this optimization available. How about naming functions that share this pattern something like <name>_clone?

@graydon
Contributor
graydon commented Apr 22, 2013

ah! good point. yes. I am not sure what the fate of the term "copy" is in the world of "clone", but either term is fine by me. Consistency with whatever else is going on in clone-vs-copy terminology seems best to me.

@kud1ing
Contributor
kud1ing commented Apr 25, 2013

Another option: clone_filter() which visually resembles clone().filter().

Downside: clone_filter() alphabetically is more distant from filter(), this could be compensated with a see foo documentation line.

But i think i'd prefer filter_clone() or something more sentence-like like filter_on_clone()

@thestinger
Contributor

The method that's actually on vectors should just consume them and remove the elements in-place.

There's a generic filter function in the iterator module for external iterators and there will be a generic one in the new iter module for any function that implements the for protocol. So I don't think duplicating that functionality is necessary.

@catamorphism
Contributor

Needs rebasing yet again...

@catamorphism
Contributor

Closing old PRs. Reopen or file a new one if you have time to rebase it :-)

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