Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
topoll
#2797core/muxing: Generalise
StreamMuxer::poll_address_change
topoll
#2797Changes from 5 commits
aa1a242
febd518
ce958fc
9a0eed2
fc61a68
360857b
55c0da0
d9c2e6a
1479271
8f8fa91
ee0a76c
94ae705
28a2fb7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
.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.
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.
Choose a reason for hiding this comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think the many
#[inline]
attributes are from past premature optimizations. See #897.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.