Skip to content
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.

Sign up for GitHub

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

Jump to bottom

don't clone types that are copy, round two. #68459

Merged
merged 2 commits into from
Jan 28, 2020
Merged

don't clone types that are copy, round two. #68459

merged 2 commits into from
Jan 28, 2020

Conversation

matthiaskrgr
Copy link
Member

Apparently fixing some of these issues makes clippy find even more so I did a couple of rounds now.

r? @eddyb

@bors
Copy link
Contributor

bors commented Jan 23, 2020

☔ The latest upstream changes (presumably #68474) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 23, 2020
@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
2020-01-23T08:47:00.5903346Z Prepare build directory.
2020-01-23T08:47:00.6352040Z Set build variables.
2020-01-23T08:47:00.6396913Z Download all required tasks.
2020-01-23T08:47:00.6573295Z Downloading task: Bash (3.159.3)
2020-01-23T08:48:46.4530074Z ##[warning]Failed to download task 'Bash'. Error The read operation failed, see inner exception.
2020-01-23T08:48:46.4541788Z ##[warning]Inner Exception: {ex.InnerException.Message}
2020-01-23T08:48:46.4546690Z ##[warning]Back off 29.917 seconds before retry.
2020-01-23T08:51:17.7949505Z ##[warning]Failed to download task 'Bash'. Error The read operation failed, see inner exception.
2020-01-23T08:51:17.7949814Z ##[warning]Inner Exception: {ex.InnerException.Message}
2020-01-23T08:51:17.7950491Z ##[warning]Back off 29.818 seconds before retry.
2020-01-23T08:53:14.2163279Z ##[error]The read operation failed, see inner exception.
2020-01-23T08:53:14.2271856Z ##[section]Finishing: Linux mingw-check

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc/traits/util.rs Outdated Show resolved Hide resolved
eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc_errors/emitter.rs
@@ -1179,13 +1179,13 @@ impl EmitterWriter {
let level_str = level.to_string();
// The failure note level itself does not provide any useful diagnostic information
if *level != Level::FailureNote && !level_str.is_empty() {
buffer.append(0, &level_str, Style::Level(level.clone()));
buffer.append(0, &level_str, Style::Level(*level));
Copy link
Member

Choose a reason for hiding this comment

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

You might want to leave it for another PR but level should probably be Level, not &Level.
@petrochenkov and/or @estebank might know more about what's happening in this code though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think at some point Level wasn't Copy, this is probably just a leftover from that.

eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
@@ -691,7 +691,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
let root_place_projection = self.infcx.tcx.intern_place_elems(root_place.projection);

if self.access_place_error_reported.contains(&(
Place { local: root_place.local.clone(), projection: root_place_projection },
Place { local: *root_place.local, projection: root_place_projection },
Copy link
Member

Choose a reason for hiding this comment

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

cc @matthewjasper Why is there a reference to a Local here - is it a mutable reference perhaps?

eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc_mir/borrow_check/mod.rs
@@ -883,7 +883,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// Check is_empty() first because it's the common case, and doing that
// way we avoid the clone() call.
if !self.access_place_error_reported.is_empty()
&& self.access_place_error_reported.contains(&(place_span.0.clone(), place_span.1))
&& self.access_place_error_reported.contains(&(*place_span.0, place_span.1))
Copy link
Member

Choose a reason for hiding this comment

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

@oli-obk @spastorino Maybe we need to consider passing Place by value more often?
(it's a pair of an integer and a pointer, so it should be really compact)

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't work if there are subslices of the projection involved. We do have a PlaceRef type for exactly that use case though

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, except in the cases @oli-obk mentions I'd change most of the uses to take Place by value. I need to do a bunch of cleanups still :).

@matthiaskrgr matthiaskrgr force-pushed the clone_on_copy2 branch 2 times, most recently from 4dac326 to 8cc79ab Compare January 24, 2020 10:45
eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc/mir/mod.rs Outdated
Index(()) => Index(()),
elem => elem.clone(),
elem => *elem,
Copy link
Member

Choose a reason for hiding this comment

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

This entire .map(|elem| ...) feels like .copied().
So really, projs is just self.projs.clone().

Or better, self.projs.fold_with(folder) (and derive/impl TypeFoldable on ProjectionElem if it doesn't already have that - it should, though).

eddyb
eddyb reviewed Jan 24, 2020
View reviewed changes
src/librustc/mir/mod.rs Outdated
@@ -2866,7 +2866,7 @@ impl<'tcx> TypeFoldable<'tcx> for PlaceElem<'tcx> {
Deref => Deref,
Field(f, ty) => Field(*f, ty.fold_with(folder)),
Index(v) => Index(v.fold_with(folder)),
elem => elem.clone(),
elem => *elem,
Copy link
Member

Choose a reason for hiding this comment

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

Actually, the way this is structured is probably a bad idea and should be exhaustive, hmpf.

Copy link
Member Author

Choose a reason for hiding this comment

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

--- a/src/librustc/mir/mod.rs
+++ b/src/librustc/mir/mod.rs
@@ -2466,7 +2466,13 @@ impl<'tcx> TypeFoldable<'tcx> for UserTypeProjection {
                 Deref => Deref,
                 Field(f, ()) => Field(*f, ()),
                 Index(()) => Index(()),
-                elem => *elem,
+                Downcast(symbol, variantidx) => Downcast(*symbol, *variantidx),
+                ConstantIndex { offset, min_length, from_end } => {
+                    ConstantIndex { offset: *offset, min_length: *min_length, from_end: *from_end }
+                }
+                Subslice { from, to, from_end } => {
+                    Subslice { from: *from, to: *to, from_end: *from_end }
+                }
             })
             .collect();

eddyb
eddyb reviewed Jan 26, 2020
View reviewed changes
src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
@eddyb
Copy link
Member

eddyb commented Jan 26, 2020

@bors r+ Thanks!

@bors
Copy link
Contributor

bors commented Jan 26, 2020

📌 Commit 6a722acfa9700993dabea1ae4f766cc208cbf94c has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2020
@bors
Copy link
Contributor

bors commented Jan 27, 2020

☔ The latest upstream changes (presumably #68407) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 27, 2020
@eddyb
Copy link
Member

eddyb commented Jan 27, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Jan 27, 2020

📌 Commit f7dcdcc has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2020
@bors
Copy link
Contributor

bors commented Jan 27, 2020

⌛ Testing commit f7dcdcc with merge fa4004a3d536df6cda05dd0a48c9d65eb5b40172...

@pietroalbini
Copy link
Member

@bors retry

Yielding priority to the stable release.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 27, 2020
@Mark-Simulacrum
Rollup merge of rust-lang#68459 - matthiaskrgr:clone_on_copy2, r=eddyb
aa6e58b 
don't clone types that are copy, round two.

Apparently fixing some of these issues makes clippy find even more so I did a couple of rounds now.

r? @eddyb
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jan 27, 2020
Merged
bors added a commit that referenced this pull request Jan 27, 2020
@bors
Auto merge of #68580 - Mark-Simulacrum:rollup-r80xhus, r=Mark-Simulacrum
43c5a7d 
Rollup of 3 pull requests

Successful merges:

 - #68459 (don't clone types that are copy, round two.)
 - #68576 (update miri)
 - #68579 (Update cargo)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Jan 27, 2020
@bors
Auto merge of #68580 - Mark-Simulacrum:rollup-r80xhus, r=Mark-Simulacrum
0859451 
Rollup of 3 pull requests

Successful merges:

 - #68459 (don't clone types that are copy, round two.)
 - #68576 (update miri)
 - #68579 (Update cargo)

Failed merges:

r? @ghost
@bors bors merged commit f7dcdcc into rust-lang:master Jan 28, 2020
@matthiaskrgr matthiaskrgr deleted the clone_on_copy2 branch February 29, 2020 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@CryZe CryZe CryZe left review comments

@oli-obk oli-obk oli-obk left review comments

@spastorino spastorino spastorino left review comments

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@matthiaskrgr @bors @rust-highfive @eddyb @pietroalbini @spastorino @oli-obk @CryZe @estebank