let initOnce take a shared Mutex#5661
Conversation
|
Thanks for your pull request, @aG0aep6G! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
| } | ||
|
|
||
| deprecated("use the overload that takes a `shared(Mutex)`") | ||
| auto ref initOnce(alias var)(lazy typeof(var) init, Mutex mutex) |
There was a problem hiding this comment.
Not sure about this. Should we deprecate code that's been using __gshared mutexes without issue?
There was a problem hiding this comment.
Let's not (yet). Mutex didn't work with shared at all for the longest time, and I'm not even sure how well it fits within the (theoretical) shared model.
There was a problem hiding this comment.
Let's not (yet).
Ok, how about changing the deprecation to a comment in the docs indicating that the this non-shared overload may be deprecated in the future, so to discourage the use in new code.
Mutex didn't work with shared at all for the longest time
Yeah, it was a shame. Now that dlang/druntime#1728 is merged we should be good, no?
I'm not even sure how well it fits within the (theoretical) shared model.
@klickverbot Can you take a look at http://dlang.org/phobos-prerelease/core_sync_mutex and the source and check if there's anything that can be improved w.r.t. the correct use of the shared type qualifier?
There was a problem hiding this comment.
Ok, how about changing the deprecation to a comment in the docs indicating that the this non-shared overload may be deprecated in the future
Yep, anything like that would be fine – the point is just to decouple allowing shared from forcing it, as the former is entirely uncontroversial. As for the latter, I don't currently see any issues with Mutex and shared as it stands (unless we end up moving towards more ownership-based semantics), but as long as we don't have shared nailed down, it seems unwise to break too much code that is just as valid without it.
Actually, thinking about it: Even if shared actually implied non-thread-local right now, it seems like calling initOnce with a thread-local mutex wouldn't be invalid. Requiring shared would just serve to catch bugs where a thread-local mutex is passed by accident (e.g. by leaving shared off a static variable).
There was a problem hiding this comment.
I've un-deprecated the unshared overload. Not adding a comment about future deprecation, because it's not clear (to me) what's going to happen.
aG0aep6G
force-pushed
the
initOnce-shared-Mutex
branch
from
924852a to
82fe75f
Compare
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Looks a lot nicer :)
Thanks for pushing this!
6 participants
As initiated by @ZombineDev (#4551 (comment)).
Depends on #4551 (included here) and dlang/druntime#1605.