Skip to content

Add new_count_waker #1402

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Jan 14, 2019
Merged

Add new_count_waker #1402

merged 4 commits into from
Jan 14, 2019

Conversation

Thomasdezeeuw
Copy link
Contributor

This is an alternative API to WakeCounter, see #1384.

I didn't touch the old API, but it might be a bit confusing that there are two different API's to do the same thing. I'll leave it up to someone else to decide whether to deprecate/remove/leave it.

Closes #1384.

/cc @Nemo157

@Thomasdezeeuw
Copy link
Contributor Author

Some more notes:

The atomic ordering might be overkill, but since it's a test API I don't think the performance impact matters too much. Also I didn't add a test, but the example tests the entire API already.

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

LGTM, we should settle on just a single API before 0.3 is released, but having both for now seems fine to me.

@Thomasdezeeuw
Copy link
Contributor Author

I think the Travis failure is unrelated, from the log:

error[E0463]: can't find crate for `std`
  |
  = note: the `thumbv6m-none-eabi` target may not be installed

Other architectures are fine.

@Nemo157
Copy link
Member

Nemo157 commented Jan 9, 2019

Yeah, it’s #1396, I would fix it but I don’t really know if there’s a way to do so that doesn’t break other stuff.

@taiki-e taiki-e mentioned this pull request Jan 11, 2019
This is an alternative API to WakeCounter.

Closes #1384.
@Thomasdezeeuw
Copy link
Contributor Author

I've force pushed a rebase on master, the CI should pass now as the failure was fixed in #1407.

@cramertj
Copy link
Member

Seems good to me! I'd prefer we replaced the old one at the same time, though, or at least marked it as deprecated and switched our internal usage. No reason to let tech debt sit around on an alpha version of the library.

inner: Arc<WakerInner>,
}

impl PartialEq<usize> for AwokenCount {
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 Deref<Target = usize> would also be handy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How can I do that? I don't think AtomicUsize has any methods that allow you to get a reference to the inner usize. I tried the following

impl Deref for AwokenCount {
    type Target = usize;

    fn deref(&self) -> &usize {
        &self.inner.count.load(Ordering::SeqCst)
    }
}

But that gives the cannot return reference to temporary value error, which makes sense.

I could add back the get(&self) -> usize method

Copy link
Member

Choose a reason for hiding this comment

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

ah, yeah, you're right-- get would be great.

@Thomasdezeeuw
Copy link
Contributor Author

@cramertj I'm going to address the comments now, to respond to your comment in the other issue.

@cramertj
Copy link
Member

sg, thanks!

@Thomasdezeeuw
Copy link
Contributor Author

Pushed 2 commits to changes the tests and remove the old WakeCounter. Should I implement the get(&self) -> usize method?

@cramertj
Copy link
Member

Yeah, get sounds good. Thanks!

@Thomasdezeeuw
Copy link
Contributor Author

@cramertj I think the test will pass (they work on my machine ™), if your OK with the API then I'm going to bed now.

@cramertj
Copy link
Member

Yup, looks good, thanks for the changes!

@cramertj cramertj merged commit 7f491d2 into rust-lang:master Jan 14, 2019
@Thomasdezeeuw Thomasdezeeuw deleted the awoken-count branch January 15, 2019 09:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants