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

Remove Boxing of response futures #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove Boxing of response futures #6

wants to merge 2 commits into from

Conversation

jonhoo
Copy link
Collaborator

@jonhoo jonhoo commented Sep 12, 2019

Waiting on rust-lang/rust#63063.

@@ -37,6 +37,7 @@
//! the client helper as `Client` in the protocol module you're working with (e.g.,
//! [`pipeline::Client`]), and the server helper as `Server` in the same place.
#![deny(missing_docs)]
#![feature(type_alias_impl_trait)]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you can feature flag this, a bit unfortunate since this doesn't seem like its going to hit stable anytime soon :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I really want this PR to land, but yeah, not looking to stabilize for a while. It would essentially be an unstable feature flag, which isn't great.. :/

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if you could do this with some sort of generic or something maybe but yeah, go head we can fix later but we should probably not get things stuck on the nightly drug. I just asked in zulip about this feature not gonna be anytime soon :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So you're proposing we fold this under, like feature = "nightly-nobox"? I don't think that'll work, because then the feature wouldn't be additive. If something that depends on tokio-tower without the feature somehow got compiled with the feature, it might not compile.

Copy link
Member

Choose a reason for hiding this comment

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

yeah im not sure but happy to not block this now for it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think putting a version out there with non-additive features is just asking for downstream users to have issues sadly. I think it's fine to keep the Box version for now, and then have an explicit git opt-in for those (like me) who want to squeeze the last little bit out.

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.

2 participants