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

Remove things #3

Merged
merged 2 commits into from
Mar 10, 2020
Merged

Remove things #3

merged 2 commits into from
Mar 10, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Mar 9, 2020

r? @CAD97

@CAD97
Copy link
Collaborator

CAD97 commented Mar 9, 2020

Needs to be rebased.

@CAD97
Copy link
Collaborator

CAD97 commented Mar 9, 2020

Quick lightning review before rebase:

5ed2f25: 👍
ae97120: #4
9c43d8d: 👍
2bab9f7:

We "use" the Copy bound in that we assume passing by-value is the correct way to do it. If we remove the Copy bound, TextSized::text_sized should take &self, because there's no reason for it to consume the input value. But because we want TextSize::of('🥰') to work, it should be taking impl TextSized by value. (Note that this is adding a : Sized bound on the usage there, so at the least TextSized should probably be : Sized.) So I think the best option is to make TextSized: Copy and document that it should be implemented for "text handles," i.e., free-copy references.

matklad added 2 commits March 10, 2020 11:11
The impl is valid, but probably not too useful, we can always add it later
@matklad
Copy link
Member Author

matklad commented Mar 10, 2020

Yup, reason for Copy makes a ton of sense actually, re-added the bound

@matklad matklad merged commit a9a7668 into master Mar 10, 2020
@matklad matklad deleted the small-things branch March 10, 2020 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants