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

Rename ArcWake to Wake #1784

Closed
wants to merge 1 commit into from
Closed

Rename ArcWake to Wake #1784

wants to merge 1 commit into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Aug 7, 2019

Since both methods take Arc as an argument, it should not be difficult to understand that this trait is based on Arc. Also, I think this should be near to the API we want to add to std/alloc in the future.

@cramertj
Copy link
Member

cramertj commented Aug 7, 2019

I have no other particularly strong feelings about this aside from the fact that this makes the trait sound more canonical, rather than like a convenient helper. cc @seanmonstar @carllerche who may have opinions about this.

@LucioFranco
Copy link
Member

Is making the ArcWake the Wake the right choice if down the line executors that do not need the atomic ref count?

@carllerche
Copy link
Member

I don't have any opinion. ArcWake is descriptive. It also is probably the strategy that should be used unless there is a reason not to.

@cramertj
Copy link
Member

cramertj commented Aug 7, 2019

Is making the ArcWake the Wake the right choice if down the line executors that do not need the atomic ref count?

@LucioFranco Given that the Wake trait is a convenience which needn't be used by all executors, and that many executors (most?) will, I think, need the refcount, it makes sense to me to make this ergonomic. However, I don't particularly care either way here and would be happy to defer to someone with a stronger opinion :)

@LucioFranco
Copy link
Member

Yeah, that makes sense, though I think the descriptiveness is actually nice and clear imho. There are a lot of "wake" terms flying around and I think keeping it ArcWake just makes more sense. In the end we use this only in a few spots so im not too worried. Just thought I'd leave my opinion :)

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.

4 participants