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

Add FromIterator and Extend implementations #15

Merged
merged 1 commit into from
Jul 25, 2015

Conversation

chris-morgan
Copy link
Contributor

FromIterator<T> and Extend<T> are implemented for the following values of T:

Review on Reviewable

@SimonSapin
Copy link
Member

Please use &[b] or &[*b] instead of ref_slice, like in #14. Other than this, looks good. (The third impl is in the spirit of rust-lang/rfcs#839.)

While you’re at it, you may want to also implement Extend and rewrite FromIterator in terms of it. (But it’e fine as is.)

@chris-morgan chris-morgan force-pushed the impl-fromiterator-for-tendril branch from 78a3825 to 9adfea0 Compare July 23, 2015 09:47
@chris-morgan
Copy link
Contributor Author

OK, updated to address feedback, including adding Extend implementations. Funny how I forgot about that—I had looked at the Extend<&Tendril<F>> implementation, noted its irrelevance to the task at hand and forgotten to ever come back to it.

It might be good to implement Extend<&str> for StrTendril and Extend<&[u8]> for ByteTendril, ideally through SliceFormat though we can’t at present do it thus generically, as well. What say you?

@SimonSapin
Copy link
Member

std has impl<'a> Extend<&'a str> for String, so adding it for StrTendril sounds fine. It doesn’t have the bytes or slice equivalent, but I don’t know a reason not to. So go for it if the coherence rules let you.

@bors-servo
Copy link
Contributor

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

@chris-morgan chris-morgan force-pushed the impl-fromiterator-for-tendril branch from 9adfea0 to 5771df4 Compare July 25, 2015 00:18
@chris-morgan chris-morgan changed the title impl FromIterator for Tendril Add FromIterator and Extend implementations Jul 25, 2015
@chris-morgan
Copy link
Contributor Author

I have rebased, updated and rendered everything consistent. Ready for merging, I believe.

`FromIterator<T>` and `Extend<T>` are implemented for the following
values of `T`:

- `char` for `StrTendril`;
- `u8` for `ByteTendril`;
- `&u8` for `ByteTendril`, for reasons of ergonomics (see also
  rust-lang/rfcs#839 for justification);
- `&str` for `StrTendril`;
- `&[u8]` for `ByteTendril`;
- `&Tendril<F>` for `Tendril<F>` (the `Extend` implementation already
  existed; the `FromIterator` is added).
@chris-morgan chris-morgan force-pushed the impl-fromiterator-for-tendril branch from 5771df4 to 0788b2b Compare July 25, 2015 01:06
@SimonSapin
Copy link
Member

Looks great. Thanks!

@SimonSapin
Copy link
Member

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 0788b2b has been approved by SimonSapin

@bors-servo
Copy link
Contributor

⌛ Testing commit 0788b2b with merge 37a545d...

bors-servo pushed a commit that referenced this pull request Jul 25, 2015
…onSapin

Add FromIterator and Extend implementations

`FromIterator<T>` and `Extend<T>` are implemented for the following values of `T`:

- `char` for `StrTendril`;
- `u8` for `ByteTendril`;
- `&u8` for `ByteTendril`, for reasons of ergonomics (see also rust-lang/rfcs#839 for justification);
- `&str` for `StrTendril`;
- `&[u8]` for `ByteTendril`;
- `&Tendril<F>` for `Tendril<F>` (the `Extend` implementation already existed; the `FromIterator` is added).

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

☀️ Test successful - travis

@bors-servo bors-servo merged commit 0788b2b into servo:master Jul 25, 2015
@chris-morgan chris-morgan deleted the impl-fromiterator-for-tendril branch July 28, 2015 10:16
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