-
Notifications
You must be signed in to change notification settings - Fork 999
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
feat(swarm): impl FromStr
for StreamProtocol
#3951
Conversation
Allows using `StreamProtocol` e.g. as a `clap` argument.
inner: Either::Right(Arc::from(protocol)), // FIXME: Can we somehow reuse the allocation from the owned string? | ||
inner: Either::Right(Arc::from(protocol)), |
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.
Leftover from a refactor, right @thomaseizinger?
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.
No, this still applies. Arc
will allocate again despite the String
already having an allocation.
type Err = InvalidProtocol; | ||
|
||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
Self::try_from_owned(s.into()) |
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 results in one more allocation than necessary because we could also construct an Arc<str>
from a string slice and don't have to go via an owned string.
We would duplicate the check then but perhaps you can refactor that check out in a separate function?
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.
Or perhaps we can widen the accepted type on try_from_owned
to something impl
? But then we'd have to rename it as well ..
Friendly ping @mxinden. |
Closing here. I don't have a direct need for this any longer. In case someone does, please open as a new pull request. |
Description
Allows using
StreamProtocol
e.g. as aclap
argument.Notes & open questions
Follow-up to #3746 (comment).
Change checklist