Skip to content
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

Merged
merged 13 commits into from
Aug 16, 2022

Conversation

thomaseizinger
Copy link
Contributor

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 performed a self-review of my own code
  • I have made corresponding changes to the documentation
    - [ ] I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

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 thomaseizinger mentioned this pull request Aug 3, 2022
24 tasks
core/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

core/src/muxing.rs Outdated Show resolved Hide resolved
@thomaseizinger thomaseizinger changed the title Generalise StreamMuxer::poll_address_change to poll_event core/muxing: Generalise StreamMuxer::poll_address_change to poll_event Aug 8, 2022
@thomaseizinger thomaseizinger marked this pull request as ready for review August 9, 2022 09:20
@thomaseizinger thomaseizinger changed the title core/muxing: Generalise StreamMuxer::poll_address_change to poll_event core/muxing: Generalise StreamMuxer::poll_address_change to poll Aug 9, 2022
core/src/muxing.rs Outdated Show resolved Hide resolved
}

/// An event produced by a [`StreamMuxer`].
pub enum StreamMuxerEvent {
Copy link
Contributor Author

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Sounds good to me.

Copy link
Member

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 do use muxing::Event in other files, but instead just use muxing and then refer to it as muxing::Event.

Copy link
Contributor

@elenaf9 elenaf9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

}

/// An event produced by a [`StreamMuxer`].
pub enum StreamMuxerEvent {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Sounds good to me.

@@ -65,14 +59,16 @@ where
.map_err(into_io_error)
}

fn poll_address_change(
#[inline]
Copy link
Contributor

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].

Copy link
Contributor Author

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 :)

Copy link
Member

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.

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].

Copy link
Member

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.

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.

@mxinden
Copy link
Member

mxinden commented Aug 14, 2022

If I am not mistaken this only requires the rename from StreamMuxerEvent to Event, right?

#2797 (comment)

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.)

@thomaseizinger
Copy link
Contributor Author

If I am not mistaken this only requires the rename from StreamMuxerEvent to Event, right?

#2797 (comment)

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 :)

@mxinden mxinden merged commit cef5056 into master Aug 16, 2022
@thomaseizinger thomaseizinger deleted the rename-stream-muxer-poll-address-change branch September 19, 2022 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants