-
Notifications
You must be signed in to change notification settings - Fork 466
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
timely-util: make AsyncOutputHandle::give
generic over the container builder
#27825
timely-util: make AsyncOutputHandle::give
generic over the container builder
#27825
Conversation
3de0eeb
to
3428688
Compare
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.
LGTM, leaving one comment to make the diff smaller.
@@ -19,6 +19,7 @@ use std::hash::Hash; | |||
use std::pin::Pin; | |||
use std::sync::Arc; | |||
use std::time::Instant; | |||
use timely::container::CapacityContainerBuilder; |
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.
Nit: move down to the next block.
src/timely-util/src/builder_async.rs
Outdated
{ | ||
#[allow(clippy::unused_async)] | ||
pub async fn give<C: CapabilityTrait<T>>(&mut self, cap: &C, data: D) { | ||
pub async fn give<C, D>(&mut self, cap: &C, data: D) |
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.
You could use the same trick that timely uses: Implement give
for a handle that is specialized to CapacityContainerBuilder
and generic over a container, and add a separate give_with_builder
that's implemented for handles generic over any container builder. This would remove the need for the second commit.
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.
Great idea! I actually only want give
to be generic over the container so I'll skip adding a give_with_builder
for now
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
3428688
to
ea1be38
Compare
Motivation
This PR enables using the
.give()
API for any container builder that supports it. Only the first commit needs to be reviewed, the second one is a mechanical change since more type annotations are now needed that were previously inferred.Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.