-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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 dyn to areas of core. #2538
Conversation
One more error on Nightly (2019-06-17):
Fix:
|
In my other branch @ry suggested: self.editor.add_history_entry(line.clone()); which is now reflected in this PR and rebased on master. |
I think cloning is not required because if you look at the rustyline's |
@@ -94,7 +94,7 @@ impl Repl { | |||
.editor | |||
.readline(&prompt) | |||
.map(|line| { | |||
self.editor.add_history_entry(line.as_ref()); | |||
self.editor.add_history_entry(line.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.
Hm I'm not sure what was happening on the other branch - I had to make that change to get it to compile locally... but if this is passing CI, I think it would be better left as is.
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.
Otherwise LGTM.
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.
I changed it to &line
in the other branch and you pushed a commit to line.clone()
.
@kitsonk EDIT: |
For now, we can use verbose turbofish, which works on both:
|
@95th What is wrong with clone? It works in both. Probably best not to leak the reference anyways. The lifetime of |
@kitsonk Nothing wrong. Just that the cloned instance is discarded in the method. I guess |
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.
LGTM - not sure what the deal with the as_ref is but the clone works and is very far from a hot path.
Nightly Rust complains with the following:
This patch fixes it and should be supported by earlier versions of Rust. (I had to update to a recent version of nightly because master has something that is triggering a bug in RLS, updating to a more recent version of nightly fixed that).