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

Cleanups in libp2p-core in stable-futures branch #1342

Merged
merged 2 commits into from
Dec 12, 2019

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Dec 10, 2019

Removes a few left-overs, and lots of Unpin trait bounds replaced with pin-project.

@tomaka
Copy link
Member Author

tomaka commented Dec 10, 2019

Note that, unless we do some design refactoring, the substream upgrade infrastructure has to require Unpin for the substream type, as we have to read/write to them, then give ownership to them to the upgrade function.

Considering that all the helpers on AsyncRead/AsyncWrite require the Unpin trait bound, it makes sense to be to add + Unpin wherever AsyncRead/AsyncWrite is required, and not try to do without it.

fn poll_read(self: Pin<&mut Self>, cx: &mut Context, buf: &mut [u8]) -> Poll<Result<usize, IoError>> {
#[project]
match self.project() {
EitherOutput::First(a) => AsyncRead::poll_read(a, cx, buf),
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere you switched to UFCS style. What is the motivation for this change and what is the advantage here over writing a.poll_read(cx, buf)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a habit from early days of futures-preview where it wasn't possible to write a.poll_read.

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

For what my review is worth this looks good to me.

👍 for using pin-project.

In regards to Universal Function Call Syntax: I don't have an opinion whether to use the fully qualified syntax or the short method syntax, I am for consistency though.

@tomaka tomaka merged commit a720601 into libp2p:stable-futures Dec 12, 2019
@tomaka tomaka deleted the cleanup-stable branch December 12, 2019 12:39
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.

3 participants