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

Use conversion trait #126

Merged
merged 2 commits into from
Nov 19, 2015
Merged

Use conversion trait #126

merged 2 commits into from
Nov 19, 2015

Conversation

SimonSapin
Copy link
Member

This is a breaking change:

  • Atom::from_slice(…) needs to be replaced with Atom::from(…)
  • atom.as_slice().other_method() with atom.other_method()
  • atom.as_slice() in another auto-deref context with &atom
  • atom.as_slice() in another context with either &*atom or atom.as_ref()

r? @asajeffrey

Review on Reviewable

@asajeffrey
Copy link
Member

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/atom/mod.rs, line 150 [r1] (raw file):
Why From<&'a String> rather than From<String>?


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/atom/mod.rs, line 150 [r1] (raw file):
Atom::from_slice, taking a concrete &str, could be called with a &String parameter which would auto-deref: https://github.com/rust-lang/rfcs/blob/master/text/0241-deref-conversions.md

But since the parameter of From::from is generic, that coercion doesn’t work. (At least not in current Rust.) This impl helps porting existing code that relied on auto-deref, so that Atom::from_slice(&some_string) can be replaced with Atom::from(&some_string) (only renaming the method) rather than Atom::from(&*some_string) (where an explicit * deref would have to be added). I’m not sure this impl is very useful, I wouldn’t mind removing it.

From<String> would work, but it’s less useful as it consumes the string and its memory allocation.


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


src/atom/mod.rs, line 150 [r1] (raw file):
Hmm, reviewable went and rewrote my question, which was "Why From<&'a String> rather than From<String>". I think this impl is confusing (at least it's confusing me!) and Atom::from(&*some_string) is more readable. This is just one instance of a general text-to-text conversion problem that I think we need a consistent story for, but that's not a problem for this PR!


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member Author

Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion.


src/atom/mod.rs, line 150 [r1] (raw file):
I’ve removed this impl.

(<something> tends to be interpreted as an HTML tag by the Markdown parser.)


Comments from the review on Reviewable.io

@asajeffrey
Copy link
Member

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member Author

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

📌 Commit d9f3133 has been approved by asajeffrey

@bors-servo
Copy link
Contributor

⌛ Testing commit d9f3133 with merge 5f0302c...

bors-servo pushed a commit that referenced this pull request Nov 19, 2015
Use conversion trait

This is a breaking change:

* `Atom::from_slice(…)` needs to be replaced with `Atom::from(…)`
* `atom.as_slice().other_method()` with `atom.other_method()`
* `atom.as_slice()` in another auto-deref context with `&atom`
* `atom.as_slice()` in another context with either `&*atom` or `atom.as_ref()`

r? @asajeffrey

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/string-cache/126)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - travis

@bors-servo bors-servo merged commit d9f3133 into master Nov 19, 2015
@SimonSapin SimonSapin deleted the convert-traits branch November 19, 2015 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants