Skip to content

/ rust-url

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

Derive `Copy` for some structures. #393

Merged
merged 1 commit into from Sep 28, 2017
Merged

Derive `Copy` for some structures. #393

merged 1 commit into from Sep 28, 2017

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Sep 28, 2017 •
edited by larsbergstrom

This change is Reviewable
@frewsxcv
Derive `Copy` for some structures.
Loading status checks…
efffb61
@KiChjang
Copy link
Member

KiChjang commented Sep 28, 2017

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2017

📌 Commit efffb61 has been approved by KiChjang
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2017

⌛ Testing commit efffb61 with merge 54d36fe...
bors-servo added a commit that referenced this pull request Sep 28, 2017
@bors-servo
Auto merge of #393 - frewsxcv:frewsxcv-copy, r=KiChjang
Loading status checks…
54d36fe 
Derive `Copy` for some structures.

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-url/393)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Sep 28, 2017

☀️ Test successful - status-travis
Approved by: KiChjang
Pushing 54d36fe to master...
@bors-servo bors-servo merged commit efffb61 into servo:master Sep 28, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:frewsxcv-copy branch Sep 28, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Sep 29, 2017

These types are private, and it looks like nothing uses these new impls. Why this change?
@frewsxcv
Copy link
Member Author

frewsxcv commented Sep 29, 2017

My understanding was that deriving Copy for types would allow the compiler to better optimize and allow for more inlining. But if that's not the case, let me know and I can revert
@SimonSapin
Copy link
Member

SimonSapin commented Sep 29, 2017

I hadn’t heard of that. Maybe that’s the case, but I’d be surprised. Do you have a reference for that? If the code compiled without Copy, any place it would do an implicit copy it instead did a move. And a move is implemented the same as a copy, only you’re not allowed to use the old location afterwards. But that’s only a compile-time check.
@frewsxcv
Copy link
Member Author

frewsxcv commented Oct 1, 2017

I haven't found anything concrete about performance, so I think you're right. There is this, but I'm guessing that's more of a suggestion for public types, and I didn't realize these types were not public until you mentioned it a couple comments ago.

In any case: #399
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@frewsxcv @KiChjang @bors-servo @SimonSapin
You can’t perform that action at this time.