-
Notifications
You must be signed in to change notification settings - Fork 974
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
Complements to #389 #415
Complements to #389 #415
Conversation
secio/src/stream_cipher.rs
Outdated
@@ -1,6 +1,26 @@ | |||
// Copyright 2017 Parity Technologies (UK) Ltd. |
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.
2018
secio/src/codec/mod.rs
Outdated
@@ -35,7 +35,7 @@ mod encode; | |||
/// Type returned by `full_codec`. | |||
pub type FullCodec<S> = DecoderMiddleware<EncoderMiddleware<length_delimited::Framed<S>>>; | |||
|
|||
pub type StreamCipher = StreamCipherCore; | |||
pub type StreamCipher = Box<dyn StreamCipherCore>; |
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.
Could you do Box<dyn StreamCipherCore + Send>
while you're at it? cc #369
@@ -35,7 +35,7 @@ mod encode; | |||
/// Type returned by `full_codec`. | |||
pub type FullCodec<S> = DecoderMiddleware<EncoderMiddleware<length_delimited::Framed<S>>>; | |||
|
|||
pub type StreamCipher = StreamCipherCore; | |||
pub type StreamCipher = Box<dyn StreamCipherCore + Send>; |
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.
is this a nightly thing?
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.
It was stabilized in Rust 1.27. Not using dyn
will become deprecated at the end of the year I think.
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.
Yes, according to the Rust 2018 roadmap.
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.
Should we note somewhere that we are expecting you to use latest rust stable (or at least 1.27+)?
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.
It should be in the CI if we do so.
Some cosmetic fix and the missing copyright for #386