core/muxing: Generalise StreamMuxer::poll_address_change to poll#2797
core/muxing: Generalise StreamMuxer::poll_address_change to poll#2797
StreamMuxer::poll_address_change to poll#2797Conversation
This is to allow general-purpose background work to be performed by implementations.
This is to allow any kind of background work to happen before anything else.
thomaseizinger
changed the title
poll_eventStreamMuxer::poll_address_change to poll_event
thomaseizinger
changed the title
StreamMuxer::poll_address_change to poll_eventStreamMuxer::poll_address_change to poll
| } | ||
|
|
||
| /// An event produced by a [`StreamMuxer`]. | ||
| pub enum StreamMuxerEvent { |
There was a problem hiding this comment.
Any thoughts on just naming this Event and referring to is as muxing::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.
I am in favor of just Event as long as we don't do use muxing::Event in other files, but instead just use muxing and then refer to it as muxing::Event.
| } | ||
|
|
||
| /// An event produced by a [`StreamMuxer`]. | ||
| pub enum StreamMuxerEvent { |
| } | ||
|
|
||
| fn poll_address_change( | ||
| #[inline] |
There was a problem hiding this comment.
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 #[inline].
There was a problem hiding this comment.
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.
I think the many #[inline] attributes are from past premature optimizations. See #897.
the rough advice I got was that the compiler tends to be smarter on when it is needed :)
Yes. Unless proven through a benchmark, let the compiler make the decision and thus don't use #[inline].
There was a problem hiding this comment.
Not on purpose to be honest. I thought I left everything as it was before.
The method was marked as #[inline] before this patch as well. I am fine with either removing it here or in a pull request in the future.
|
If I am not mistaken this only requires the rename from Also happy for this to happen in a different pull request if you prefer @thomaseizinger. (Sorry for the merge, thought this was already ready to go.) |
Yes. I am also happy for this to happen in a different PR :) |
4 participants
Description
This is to allow general-purpose background work to be performed
by implementations.
Links to any relevant issues
#2722
Open Questions
Change checklist
- [ ] I have added tests that prove my fix is effective or that my feature works