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

async: add DelayUs #344

Merged
merged 2 commits into from Jan 12, 2022
Merged

async: add DelayUs #344

merged 2 commits into from Jan 12, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Jan 11, 2022

Add async DelayUs, mirroring the blocking one.

Unlike blocking, delay_ms has no default impl because it's not possible to have default impls in GAT-based async traits.

@Dirbaio Dirbaio requested a review from a team as a code owner January 11, 2022 22:24
@rust-highfive
Copy link

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

@Dirbaio Dirbaio mentioned this pull request Jan 11, 2022
3 tasks
@Dirbaio Dirbaio changed the title Async delay async: add DelayUs Jan 11, 2022
@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 11, 2022

How do we handle the changelog check for the async crate? No need to changelog anything until first release, I guess.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Seems fine to me, thanks!
Do you really need two different futures for the us and ms variants?

The changelog check does not seem to support more than one changelog file in a repository so I think we can just ignore it for e-h-a changes. It is not required for a bors merge, just slightly annoying for maintainers.

@Dirbaio
Copy link
Member Author

Dirbaio commented Jan 12, 2022

Do you really need two different futures for the us and ms variants?

an impl may want to have a loop for the ms variant like in the blocking default impl, to avoid overflow. In that case they'd be different futures.

Also, for consistency I'd rather always do "1 method = 1 future associated type". This is what the future "async fn in traits" will desugar to. Also, users will almost never use the associated types, they'll always do foo.method().await, so having many of them is not bad for usability. This gives max flexibility to implementors with no downside.

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

I see, thanks for the clarification.
Looks good to me!
bors r+

@bors bors bot merged commit 9c17bca into rust-embedded:master Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants