Skip to content

Opt for .cloned() over .map(|foo| foo.clone()) etc. #22287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 19, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/compiletest/compiletest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#![feature(test)]
#![feature(unicode)]
#![feature(env)]
#![feature(core)]

#![deny(warnings)]

Expand Down
2 changes: 1 addition & 1 deletion src/compiletest/runtest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1133,7 +1133,7 @@ fn compile_test_(config: &Config, props: &TestProps,
// FIXME (#9639): This needs to handle non-utf8 paths
let mut link_args = vec!("-L".to_string(),
aux_dir.as_str().unwrap().to_string());
link_args.extend(extra_args.iter().map(|s| s.clone()));
link_args.extend(extra_args.iter().cloned());
let args = make_compile_args(config,
props,
link_args,
Expand Down
6 changes: 3 additions & 3 deletions src/libcollections/bit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2306,7 +2306,7 @@ mod tests {
#[test]
fn test_from_bools() {
let bools = vec![true, false, true, true];
let bitv: Bitv = bools.iter().map(|n| *n).collect();
let bitv: Bitv = bools.iter().cloned().collect();
assert_eq!(format!("{:?}", bitv), "1011");
}

Expand All @@ -2319,12 +2319,12 @@ mod tests {
#[test]
fn test_bitv_iterator() {
let bools = vec![true, false, true, true];
let bitv: Bitv = bools.iter().map(|n| *n).collect();
let bitv: Bitv = bools.iter().cloned().collect();

assert_eq!(bitv.iter().collect::<Vec<bool>>(), bools);

let long: Vec<_> = (0i32..10000).map(|i| i % 2 == 0).collect();
let bitv: Bitv = long.iter().map(|n| *n).collect();
let bitv: Bitv = long.iter().cloned().collect();
assert_eq!(bitv.iter().collect::<Vec<bool>>(), long)
}

Expand Down
4 changes: 2 additions & 2 deletions src/libcollections/dlist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,7 @@ impl<A: Ord> Ord for DList<A> {
#[stable(feature = "rust1", since = "1.0.0")]
impl<A: Clone> Clone for DList<A> {
fn clone(&self) -> DList<A> {
self.iter().map(|x| x.clone()).collect()
self.iter().cloned().collect()
}
}

Expand Down Expand Up @@ -1056,7 +1056,7 @@ mod tests {

#[cfg(test)]
fn list_from<T: Clone>(v: &[T]) -> DList<T> {
v.iter().map(|x| (*x).clone()).collect()
v.iter().cloned().collect()
}

#[test]
Expand Down
27 changes: 22 additions & 5 deletions src/libcore/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ pub trait IteratorExt: Iterator + Sized {
///
/// ```
/// let xs = [100, 200, 300];
/// let mut it = xs.iter().map(|x| *x).peekable();
/// let mut it = xs.iter().cloned().peekable();
/// assert_eq!(*it.peek().unwrap(), 100);
/// assert_eq!(it.next().unwrap(), 100);
/// assert_eq!(it.next().unwrap(), 200);
Expand Down Expand Up @@ -540,7 +540,7 @@ pub trait IteratorExt: Iterator + Sized {
///
/// let a = [1, 4, 2, 3, 8, 9, 6];
/// let sum = a.iter()
/// .map(|&x| x)
/// .cloned()
/// .inspect(|&x| println!("filtering {}", x))
/// .filter(|&x| x % 2 == 0)
/// .inspect(|&x| println!("{} made it through", x))
Expand Down Expand Up @@ -579,7 +579,7 @@ pub trait IteratorExt: Iterator + Sized {
///
/// ```
/// let a = [1, 2, 3, 4, 5];
/// let b: Vec<_> = a.iter().map(|&x| x).collect();
/// let b: Vec<_> = a.iter().cloned().collect();
/// assert_eq!(a, b);
/// ```
#[inline]
Expand Down Expand Up @@ -955,7 +955,7 @@ pub trait IteratorExt: Iterator + Sized {
///
/// ```
/// let a = [(1, 2), (3, 4)];
/// let (left, right): (Vec<_>, Vec<_>) = a.iter().map(|&x| x).unzip();
/// let (left, right): (Vec<_>, Vec<_>) = a.iter().cloned().unzip();
/// assert_eq!([1, 3], left);
/// assert_eq!([2, 4], right);
/// ```
Expand Down Expand Up @@ -1160,7 +1160,7 @@ pub trait AdditiveIterator<A> {
/// use std::iter::AdditiveIterator;
///
/// let a = [1i32, 2, 3, 4, 5];
/// let mut it = a.iter().map(|&x| x);
/// let mut it = a.iter().cloned();
/// assert!(it.sum() == 15);
/// ```
fn sum(self) -> A;
Expand Down Expand Up @@ -1323,6 +1323,23 @@ impl<T, D, I> ExactSizeIterator for Cloned<I> where
I: ExactSizeIterator<Item=D>,
{}

#[unstable(feature = "core", reason = "trait is experimental")]
impl<T, D, I> RandomAccessIterator for Cloned<I> where
T: Clone,
D: Deref<Target=T>,
I: RandomAccessIterator<Item=D>
{
#[inline]
fn indexable(&self) -> usize {
self.it.indexable()
}

#[inline]
fn idx(&mut self, index: usize) -> Option<T> {
self.it.idx(index).cloned()
}
}

/// An iterator that repeats endlessly
#[derive(Clone)]
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
Expand Down
44 changes: 22 additions & 22 deletions src/libcoretest/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn test_iterator_chain() {
assert_eq!(i, expected.len());

let ys = count(30, 10).take(4);
let mut it = xs.iter().map(|&x| x).chain(ys);
let mut it = xs.iter().cloned().chain(ys);
let mut i = 0;
for x in it {
assert_eq!(x, expected[i]);
Expand Down Expand Up @@ -119,7 +119,7 @@ fn test_iterator_enumerate() {
#[test]
fn test_iterator_peekable() {
let xs = vec![0, 1, 2, 3, 4, 5];
let mut it = xs.iter().map(|&x|x).peekable();
let mut it = xs.iter().cloned().peekable();

assert_eq!(it.len(), 6);
assert_eq!(it.peek().unwrap(), &0);
Expand Down Expand Up @@ -259,7 +259,7 @@ fn test_inspect() {
let mut n = 0;

let ys = xs.iter()
.map(|&x| x)
.cloned()
.inspect(|_| n += 1)
.collect::<Vec<uint>>();

Expand Down Expand Up @@ -329,33 +329,33 @@ fn test_iterator_len() {
#[test]
fn test_iterator_sum() {
let v: &[i32] = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
assert_eq!(v[..4].iter().map(|&x| x).sum(), 6);
assert_eq!(v.iter().map(|&x| x).sum(), 55);
assert_eq!(v[..0].iter().map(|&x| x).sum(), 0);
assert_eq!(v[..4].iter().cloned().sum(), 6);
assert_eq!(v.iter().cloned().sum(), 55);
assert_eq!(v[..0].iter().cloned().sum(), 0);
}

#[test]
fn test_iterator_product() {
let v: &[i32] = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
assert_eq!(v[..4].iter().map(|&x| x).product(), 0);
assert_eq!(v[1..5].iter().map(|&x| x).product(), 24);
assert_eq!(v[..0].iter().map(|&x| x).product(), 1);
assert_eq!(v[..4].iter().cloned().product(), 0);
assert_eq!(v[1..5].iter().cloned().product(), 24);
assert_eq!(v[..0].iter().cloned().product(), 1);
}

#[test]
fn test_iterator_max() {
let v: &[_] = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
assert_eq!(v[..4].iter().map(|&x| x).max(), Some(3));
assert_eq!(v.iter().map(|&x| x).max(), Some(10));
assert_eq!(v[..0].iter().map(|&x| x).max(), None);
assert_eq!(v[..4].iter().cloned().max(), Some(3));
assert_eq!(v.iter().cloned().max(), Some(10));
assert_eq!(v[..0].iter().cloned().max(), None);
}

#[test]
fn test_iterator_min() {
let v: &[_] = &[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
assert_eq!(v[..4].iter().map(|&x| x).min(), Some(0));
assert_eq!(v.iter().map(|&x| x).min(), Some(0));
assert_eq!(v[..0].iter().map(|&x| x).min(), None);
assert_eq!(v[..4].iter().cloned().min(), Some(0));
assert_eq!(v.iter().cloned().min(), Some(0));
assert_eq!(v[..0].iter().cloned().min(), None);
}

#[test]
Expand All @@ -373,7 +373,7 @@ fn test_iterator_size_hint() {
assert_eq!(c.clone().take_while(|_| false).size_hint(), (0, None));
assert_eq!(c.clone().skip_while(|_| false).size_hint(), (0, None));
assert_eq!(c.clone().enumerate().size_hint(), (uint::MAX, None));
assert_eq!(c.clone().chain(vi.clone().map(|&i| i)).size_hint(), (uint::MAX, None));
assert_eq!(c.clone().chain(vi.clone().cloned()).size_hint(), (uint::MAX, None));
assert_eq!(c.clone().zip(vi.clone()).size_hint(), (10, Some(10)));
assert_eq!(c.clone().scan(0, |_,_| Some(0)).size_hint(), (0, None));
assert_eq!(c.clone().filter(|_| false).size_hint(), (0, None));
Expand All @@ -398,7 +398,7 @@ fn test_iterator_size_hint() {
#[test]
fn test_collect() {
let a = vec![1, 2, 3, 4, 5];
let b: Vec<int> = a.iter().map(|&x| x).collect();
let b: Vec<int> = a.iter().cloned().collect();
assert!(a == b);
}

Expand Down Expand Up @@ -471,7 +471,7 @@ fn test_rev() {
let mut it = xs.iter();
it.next();
it.next();
assert!(it.rev().map(|&x| x).collect::<Vec<int>>() ==
assert!(it.rev().cloned().collect::<Vec<int>>() ==
vec![16, 14, 12, 10, 8, 6]);
}

Expand Down Expand Up @@ -508,7 +508,7 @@ fn test_double_ended_map() {
#[test]
fn test_double_ended_enumerate() {
let xs = [1, 2, 3, 4, 5, 6];
let mut it = xs.iter().map(|&x| x).enumerate();
let mut it = xs.iter().cloned().enumerate();
assert_eq!(it.next(), Some((0, 1)));
assert_eq!(it.next(), Some((1, 2)));
assert_eq!(it.next_back(), Some((5, 6)));
Expand All @@ -522,8 +522,8 @@ fn test_double_ended_enumerate() {
fn test_double_ended_zip() {
let xs = [1, 2, 3, 4, 5, 6];
let ys = [1, 2, 3, 7];
let a = xs.iter().map(|&x| x);
let b = ys.iter().map(|&x| x);
let a = xs.iter().cloned();
let b = ys.iter().cloned();
let mut it = a.zip(b);
assert_eq!(it.next(), Some((1, 1)));
assert_eq!(it.next(), Some((2, 2)));
Expand Down Expand Up @@ -713,7 +713,7 @@ fn test_random_access_inspect() {
fn test_random_access_map() {
let xs = [1, 2, 3, 4, 5];

let mut it = xs.iter().map(|x| *x);
let mut it = xs.iter().cloned();
assert_eq!(xs.len(), it.indexable());
for (i, elt) in xs.iter().enumerate() {
assert_eq!(Some(*elt), it.idx(i));
Expand Down
4 changes: 2 additions & 2 deletions src/librand/isaac.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl<'a> SeedableRng<&'a [u32]> for IsaacRng {
fn reseed(&mut self, seed: &'a [u32]) {
// make the seed into [seed[0], seed[1], ..., seed[seed.len()
// - 1], 0, 0, ...], to fill rng.rsl.
let seed_iter = seed.iter().map(|&x| x).chain(repeat(0u32));
let seed_iter = seed.iter().cloned().chain(repeat(0u32));

for (rsl_elem, seed_elem) in self.rsl.iter_mut().zip(seed_iter) {
*rsl_elem = seed_elem;
Expand Down Expand Up @@ -458,7 +458,7 @@ impl<'a> SeedableRng<&'a [u64]> for Isaac64Rng {
fn reseed(&mut self, seed: &'a [u64]) {
// make the seed into [seed[0], seed[1], ..., seed[seed.len()
// - 1], 0, 0, ...], to fill rng.rsl.
let seed_iter = seed.iter().map(|&x| x).chain(repeat(0u64));
let seed_iter = seed.iter().cloned().chain(repeat(0u64));

for (rsl_elem, seed_elem) in self.rsl.iter_mut().zip(seed_iter) {
*rsl_elem = seed_elem;
Expand Down
5 changes: 2 additions & 3 deletions src/librustc/metadata/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,7 @@ impl CStore {
pub fn get_used_crate_source(&self, cnum: ast::CrateNum)
-> Option<CrateSource> {
self.used_crate_sources.borrow_mut()
.iter().find(|source| source.cnum == cnum)
.map(|source| source.clone())
.iter().find(|source| source.cnum == cnum).cloned()
}

pub fn reset(&self) {
Expand Down Expand Up @@ -218,7 +217,7 @@ impl CStore {

pub fn find_extern_mod_stmt_cnum(&self, emod_id: ast::NodeId)
-> Option<ast::CrateNum> {
self.extern_mod_crate_map.borrow().get(&emod_id).map(|x| *x)
self.extern_mod_crate_map.borrow().get(&emod_id).cloned()
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/check_match.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ impl<'a> fmt::Debug for Matrix<'a> {
pretty_printed_matrix.iter().map(|row| row[col].len()).max().unwrap_or(0)
}).collect();

let total_width = column_widths.iter().map(|n| *n).sum() + column_count * 3 + 1;
let total_width = column_widths.iter().cloned().sum() + column_count * 3 + 1;
let br = repeat('+').take(total_width).collect::<String>();
try!(write!(f, "{}\n", br));
for row in pretty_printed_matrix {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ fn lit_to_const(lit: &ast::Lit, ty_hint: Option<Ty>) -> const_val {
match lit.node {
ast::LitStr(ref s, _) => const_str((*s).clone()),
ast::LitBinary(ref data) => {
const_binary(Rc::new(data.iter().map(|x| *x).collect()))
const_binary(data.clone())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has slightly different semantics from the above code. I'm not clear what was intended. Naively this should be a strict improvement? (just bumps a refcount instead of creating a new Rc and new Vec)

librustc is crazy, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, assumed safe rust usage which makes this equivalent? Tests passed for a full make check so it should be fine?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an Rc happens to be unique, you can actually move the data out. So there is actually a semantic change in safe Rust to make a new disjoint Rc.

I don't think rustc ever actually does this, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that just CoW? Or has it changed recently? Wouldn't the underlying Vec be the same and whatever code wants to act on a mutation would Cow or clone the inside? It's not a RefCell so doing anything strange to the underlying data would be pretty dodgy? (Am I overlooking something really terrible?)

Other than this concern, rebase is done :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to future: Talked to eddyb, this is just a case of gradual code refactoring making previously sane code insane.

}
ast::LitByte(n) => const_uint(n as u64),
ast::LitChar(n) => const_uint(n as u64),
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/middle/dataflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ struct PropagationContext<'a, 'b: 'a, 'tcx: 'b, O: 'a> {
}

fn to_cfgidx_or_die(id: ast::NodeId, index: &NodeMap<CFGIndex>) -> CFGIndex {
let opt_cfgindex = index.get(&id).map(|&i|i);
let opt_cfgindex = index.get(&id).cloned();
opt_cfgindex.unwrap_or_else(|| {
panic!("nodeid_to_index does not have entry for NodeId {}", id);
})
Expand Down Expand Up @@ -400,7 +400,7 @@ impl<'a, 'tcx, O:DataFlowOperator> DataFlowContext<'a, 'tcx, O> {

let mut changed = false;
for &node_id in &edge.data.exiting_scopes {
let opt_cfg_idx = self.nodeid_to_index.get(&node_id).map(|&i|i);
let opt_cfg_idx = self.nodeid_to_index.get(&node_id).cloned();
match opt_cfg_idx {
Some(cfg_idx) => {
let (start, end) = self.compute_id_range(cfg_idx);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/dependency_format.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ fn calculate_type(sess: &session::Session,

// Collect what we've got so far in the return vector.
let mut ret = (1..sess.cstore.next_crate_num()).map(|i| {
match formats.get(&i).map(|v| *v) {
match formats.get(&i).cloned() {
v @ Some(cstore::RequireDynamic) => v,
_ => None,
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/infer/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ impl<'a, 'tcx> Rebuilder<'a, 'tcx> {

fn rebuild(&self)
-> (ast::FnDecl, Option<ast::ExplicitSelf_>, ast::Generics) {
let mut expl_self_opt = self.expl_self_opt.map(|x| x.clone());
let mut expl_self_opt = self.expl_self_opt.cloned();
let mut inputs = self.fn_decl.inputs.clone();
let mut output = self.fn_decl.output.clone();
let mut ty_params = self.generics.ty_params.clone();
Expand Down
16 changes: 5 additions & 11 deletions src/librustc/middle/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,18 +147,12 @@ struct LanguageItemCollector<'a> {

impl<'a, 'v> Visitor<'v> for LanguageItemCollector<'a> {
fn visit_item(&mut self, item: &ast::Item) {
match extract(&item.attrs) {
Some(value) => {
let item_index = self.item_refs.get(&value[]).map(|x| *x);

match item_index {
Some(item_index) => {
self.collect_item(item_index, local_def(item.id), item.span)
}
None => {}
}
if let Some(value) = extract(&item.attrs) {
let item_index = self.item_refs.get(&value[]).cloned();

if let Some(item_index) = item_index {
self.collect_item(item_index, local_def(item.id), item.span)
}
None => {}
}

visit::walk_item(self, item);
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ impl RegionMaps {

pub fn opt_encl_scope(&self, id: CodeExtent) -> Option<CodeExtent> {
//! Returns the narrowest scope that encloses `id`, if any.
self.scope_map.borrow().get(&id).map(|x| *x)
self.scope_map.borrow().get(&id).cloned()
}

#[allow(dead_code)] // used in middle::cfg
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/resolve_lifetime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ pub fn early_bound_lifetimes<'a>(generics: &'a ast::Generics) -> Vec<ast::Lifeti

generics.lifetimes.iter()
.filter(|l| referenced_idents.iter().any(|&i| i == l.lifetime.name))
.map(|l| (*l).clone())
.cloned()
.collect()
}

Expand Down
2 changes: 1 addition & 1 deletion src/librustc/middle/subst.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl<'tcx> Substs<'tcx> {
}

pub fn self_ty(&self) -> Option<Ty<'tcx>> {
self.types.get_self().map(|&t| t)
self.types.get_self().cloned()
}

pub fn with_self_ty(&self, self_ty: Ty<'tcx>) -> Substs<'tcx> {
Expand Down
Loading