Skip to content

Cleanup #1457

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

Merged
merged 2 commits into from
Feb 20, 2019
Merged

Cleanup #1457

merged 2 commits into from
Feb 20, 2019

Conversation

cramertj
Copy link
Member

Fixes some leftover bits from #1445

@cramertj cramertj requested review from Nemo157 and matthewjasper and removed request for matthewjasper February 20, 2019 23:05
@cramertj
Copy link
Member Author

cramertj commented Feb 20, 2019

cc @Matthias247, @seanmonstar

@cramertj cramertj merged commit 79d667a into rust-lang:master Feb 20, 2019
@cramertj cramertj deleted the cleanup branch February 20, 2019 23:35
}
};
}
#[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

I doubt this has any effect, since the method is always virtually dispatched.

unsafe fn increase_refcount<T: ArcWake>(data: *const()) {
// Retain Arc by creating a copy
let arc: Arc<T> = Arc::from_raw(data as *const T);
let arc_clone = arc.clone();
// Forget the Arcs again, so that the refcount isn't decrased
let _ = Arc::into_raw(arc);
let _ = Arc::into_raw(arc_clone);
mem::forget(arc);
Copy link
Contributor

Choose a reason for hiding this comment

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

I used Arc::into_raw since that's the version where the API guarantees that it's the opposite of Arc::from_raw.
forget will work too, but I thought if there is a more specific API available it might be nice to use it.
Anyway, both is fine.

}

unsafe fn clone_arc_raw<T: ArcWake>(data: *const()) -> RawWaker {
// used by `waker_ref`
pub(super) unsafe fn clone_arc_raw<T: ArcWake>(data: *const()) -> RawWaker {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are those things exported to now? Only one level above, or even further?
It's totally to fine to use those within other parts of futures-util, but I don't think the library should publicly export these.

Copy link
Member Author

Choose a reason for hiding this comment

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

They're just pub(super)-- so only available to the futures_util::task module.

// Lazily create the `Arc` only when the waker is actually cloned.
// FIXME: remove `transmute` when a `Waker` -> `RawWaker` conversion
// function is landed in `core`.
mem::transmute::<task03::Waker, RawWaker>(
Copy link
Contributor

Choose a reason for hiding this comment

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

A method on ArcWake or the same module that returns a RawWaker seems cleaner than transmute.

The ideal solution might be to directly reference and reuse a refcounted thing within Task01. But after looking up the definition of that I don't think it's possible without the extra allocation and wrapping.

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