-
Notifications
You must be signed in to change notification settings - Fork 92
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
Prefer Clone to Copy. Fixes #74. #76
Conversation
Clone is a super-type of Copy, so everything that implements Copy already implements Clone. Therefore, this is strictly more general. In addition, .clone() on things that implement Copy should be inlined (to `return *self`) and essentially zero cost. And per Marwes#74 (comment), .clone() already be fast in order for parsing to be fast, so the performance assumptions in the non-Copy case should be similar.
@@ -189,7 +189,7 @@ pub fn satisfy_map<I, P, R>(predicate: P) -> SatisfyMap<I, P> | |||
#[derive(Clone)] | |||
pub struct Token<I> | |||
where I: Stream, | |||
I::Item: PartialEq | |||
I::Item: PartialEq + Clone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This bound and then one below it is not necessary.
{ | ||
Token { | ||
c: c, | ||
c: c.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clone is not necessary
@@ -225,10 +225,10 @@ impl<I> Parser for Token<I> | |||
#[inline(always)] | |||
pub fn token<I>(c: I::Item) -> Token<I> | |||
where I: Stream, | |||
I::Item: PartialEq | |||
I::Item: PartialEq + Clone, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary bound
Nice! Thanks! I originally limited the |
9a32f1a
to
c308819
Compare
…arwes#74. This test verifies we can use token() with a StreamSlice.
@Marwes thanks for the quick response. I turned the last commit into a test-only change. Is there anything else you want in order to merge? |
Nope, looks good now. Published to crates.io as 2.1.1. |
Together, these commits address the cases I asked about in #74 and #75. With these,
SliceStream
consumers can usetoken()
; and consumers who want aT
item type can parse non-Copy
tokens.