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
core/muxing: Generalise StreamMuxer::poll_address_change
to poll
#2797
Changes from all commits
aa1a242
febd518
ce958fc
9a0eed2
fc61a68
360857b
55c0da0
d9c2e6a
1479271
8f8fa91
ee0a76c
94ae705
28a2fb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
use crate::StreamMuxer; | ||
use crate::muxing::{StreamMuxer, StreamMuxerEvent}; | ||
use futures::{AsyncRead, AsyncWrite}; | ||
use multiaddr::Multiaddr; | ||
use pin_project::pin_project; | ||
use std::error::Error; | ||
use std::fmt; | ||
|
@@ -38,11 +37,6 @@ where | |
type Substream = SubstreamBox; | ||
type Error = io::Error; | ||
|
||
#[inline] | ||
fn poll_close(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
self.project().inner.poll_close(cx).map_err(into_io_error) | ||
} | ||
|
||
fn poll_inbound( | ||
self: Pin<&mut Self>, | ||
cx: &mut Context<'_>, | ||
|
@@ -65,14 +59,16 @@ where | |
.map_err(into_io_error) | ||
} | ||
|
||
fn poll_address_change( | ||
#[inline] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why inline this method but not the other ones, and why only inline it in this muxer? Just asking out of curiosity because I am not really familiar with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not on purpose to be honest. I thought I left everything as it was before. I am not too familiar with inlining either but the rough advice I got was that the compiler tends to be smarter on when it is needed :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the many
Yes. Unless proven through a benchmark, let the compiler make the decision and thus don't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The method was marked as |
||
fn poll_close(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
self.project().inner.poll_close(cx).map_err(into_io_error) | ||
} | ||
|
||
fn poll( | ||
self: Pin<&mut Self>, | ||
cx: &mut Context<'_>, | ||
) -> Poll<Result<Multiaddr, Self::Error>> { | ||
self.project() | ||
.inner | ||
.poll_address_change(cx) | ||
.map_err(into_io_error) | ||
) -> Poll<Result<StreamMuxerEvent, Self::Error>> { | ||
self.project().inner.poll(cx).map_err(into_io_error) | ||
} | ||
} | ||
|
||
|
@@ -109,11 +105,6 @@ impl StreamMuxer for StreamMuxerBox { | |
type Substream = SubstreamBox; | ||
type Error = io::Error; | ||
|
||
#[inline] | ||
fn poll_close(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
self.project().poll_close(cx) | ||
} | ||
|
||
fn poll_inbound( | ||
self: Pin<&mut Self>, | ||
cx: &mut Context<'_>, | ||
|
@@ -128,11 +119,16 @@ impl StreamMuxer for StreamMuxerBox { | |
self.project().poll_outbound(cx) | ||
} | ||
|
||
fn poll_address_change( | ||
#[inline] | ||
fn poll_close(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> { | ||
self.project().poll_close(cx) | ||
} | ||
|
||
fn poll( | ||
self: Pin<&mut Self>, | ||
cx: &mut Context<'_>, | ||
) -> Poll<Result<Multiaddr, Self::Error>> { | ||
self.project().poll_address_change(cx) | ||
) -> Poll<Result<StreamMuxerEvent, Self::Error>> { | ||
self.project().poll(cx) | ||
} | ||
} | ||
|
||
|
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.
Any thoughts on just naming this
Event
and referring to is asmuxing::Event
?I think there was a loose consensus around #2217 at some point but we haven't really made progress on this front.
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.
👍 Sounds good to me.
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 am in favor of just
Event
as long as we don't douse muxing::Event
in other files, but instead justuse muxing
and then refer to it asmuxing::Event
.