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.

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

[experiment] Add MustClone and impl Copy+MustClone on Range{,From,Inclusive}. #77180

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Sep 25, 2020

Background: (second half of) rust-lang/rfcs#2848 (comment).

I'm opening this for two reasons:

  • demonstrating the (relatively simple) implementation (though it could use specialized diagnostics)
  • doing a crater run just in case simply having Range<_>: Copy breaks something through e.g. inference

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2020
@eddyb
Copy link
Member Author

eddyb commented Sep 25, 2020

@bors try

@bors
Copy link
Contributor

bors commented Sep 25, 2020

⌛ Trying commit 6cdd05313a732178776903a10d41edf154eea341 with merge 715ad67e8b83d57e72e8d58387d01d804162f99a...

@eddyb
Copy link
Member Author

eddyb commented Sep 25, 2020

Oh and since this may require a fast path to not regress perf: @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2020
@eddyb
Copy link
Member Author

eddyb commented Sep 25, 2020

@bors try

@bors
Copy link
Contributor

bors commented Sep 25, 2020

⌛ Trying commit 346ede0 with merge 6dab9ef81bafc73ee769b24a6ad263eca85520b4...

@bors
Copy link
Contributor

bors commented Sep 25, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 6dab9ef81bafc73ee769b24a6ad263eca85520b4 (6dab9ef81bafc73ee769b24a6ad263eca85520b4)

@rust-timer
Copy link
Collaborator

Queued 6dab9ef81bafc73ee769b24a6ad263eca85520b4 with parent 521d8d8, future comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Sep 25, 2020

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-77180 created and queued.
🤖 Automatically detected try build 6dab9ef81bafc73ee769b24a6ad263eca85520b4
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 25, 2020
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6dab9ef81bafc73ee769b24a6ad263eca85520b4): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@craterbot
Copy link
Collaborator

🚧 Experiment pr-77180 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@petrochenkov
Copy link
Contributor

I like the general direction here, but I also agree with rust-lang/rfcs#2848 (comment) that it feels more like an "attribute + lint" case rather than a "trait + move error (?)", especially given that the trait is not used in generic context, only on concrete instances of ranges.

@craterbot
Copy link
Collaborator

🎉 Experiment pr-77180 is completed!
📊 10 regressed and 7 fixed (123440 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Sep 30, 2020
@craterbot craterbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 30, 2020
@petrochenkov
Copy link
Contributor

One root regression due to #![deny(missing_copy_implementations)], no regressions due to Copy bounds.
Other regressions are spurious.

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 30, 2020
@eddyb
Copy link
Member Author

eddyb commented Oct 1, 2020

Thanks! At least it being a lint means it shouldn't impact any crates that depend on conrod_core / kiss3d_conrod.
And having that lint trip is a nice way to confirm that something is different.

So we should be able to proceed here with an RFC or similar - though at the moment I don't have the bandwidth for anything like that, but if someone is interested in writing a #[must_clone] / trait MustClone RFC, feel free to show me drafts etc.

@eddyb
Copy link
Member Author

eddyb commented Oct 1, 2020

I'm closing this experiment PR, please continue discussion over at rust-lang/rfcs#2848 (comment), to avoid fragmentation.

@eddyb eddyb closed this Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants