Opt for .cloned() over .map(|foo| foo.clone()) etc. #22287
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
Looks good to me! |
|
@bors r+ b042d10 |
|
🙀 You have the wrong number! Please try again with |
|
@bors r+ 43f0fbf |
|
@nikomatsakis It missed |
|
@bors r- |
|
I'm canceling the r+ until the other PRs land so we can sort this out in a more orderly fashion. |
|
@bors: r- |
Ryman
force-pushed
the
purge_carthographers
branch
from
43f0fbf to
d786e10
Compare
|
Rebased on master and added the remaining cases. |
| @@ -600,7 +600,7 @@ pub fn lit_to_const(lit: &ast::Lit) -> 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()) | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
Yeah, assumed safe rust usage which makes this equivalent? Tests passed for a full make check so it should be fine?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Note to future: Talked to eddyb, this is just a case of gradual code refactoring making previously sane code insane.
|
Wow, this is way more principled than a lot of bulk refactors we get! Actually reflecting on if the surrounding code makes sense?? 👍 Just a few minor issues. |
Ryman
force-pushed
the
purge_carthographers
branch
from
d786e10 to
a11e900
Compare
|
@bors r+ a11e900 |
|
⌛ Testing commit a11e900 with merge 9c9711b... |
|
💔 Test failed - auto-win-32-nopt-t |
Ryman
force-pushed
the
purge_carthographers
branch
from
a11e900 to
fe99ca9
Compare
|
Windows failed with: @gankro re r? Rebased and reverted the above in last added commit as I don't think I can implement Clone for that? Related: are we ever going to have Copy implements Clone or was that written off for some reason? |
|
Something Something Coherence makes the blanket impl hard to do, I think. |
|
Can you squash the revert into whatever commit added it? |
Ryman
force-pushed
the
purge_carthographers
branch
from
fe99ca9 to
20db2c9
Compare
|
@gankro done. |
|
@bors r+ 20db2c |
|
Thanks a ton! |
Ryman
force-pushed
the
purge_carthographers
branch
from
20db2c9 to
d2f54e6
Compare
|
@gankro rebased as it seems to have dropped off bors's radar according to the build queue. |
|
@bors r+ d2f54e |
|
⌛ Testing commit d2f54e6 with merge 3d01ccc... |
|
💔 Test failed - auto-win-64-opt |
|
@bors: retry |
|
⌛ Testing commit d2f54e6 with merge dfaf453... |
|
💔 Test failed - auto-win-32-nopt-t |
|
@bors retry |
This overlaps with rust-lang#22276 (I left make check running overnight) but covers a number of additional cases and has a few rewrites where the clones are not even necessary. This also implements `RandomAccessIterator` for `iter::Cloned` cc @steveklabnik, you may want to glance at this before rust-lang#22281 gets the bors treatment
This overlaps with #22276 (I left make check running overnight) but covers a number of additional cases and has a few rewrites where the clones are not even necessary.
This also implements
RandomAccessIteratorforiter::Clonedcc @steveklabnik, you may want to glance at this before #22281 gets the bors treatment