Fix Issue 17806 - processAllocator getter will override set value if … #6413
Conversation
|
Thanks for your pull request and interest in making D better, @edi33416! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#6413" |
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Please don't introduce new module constructors. A lot of recent work has tried to remove them (for the reasons you see in the CI failures)
std/experimental/allocator/package.d
Outdated
| _processAllocator = sharedAllocatorObject(GCAllocator.instance); | ||
| } | ||
|
|
||
| unittest |
std/experimental/allocator/package.d
Outdated
| public import std.experimental.allocator.common, | ||
| std.experimental.allocator.typed; | ||
|
|
||
| shared static this() |
There was a problem hiding this comment.
object.Error@src/rt/minfo.d(371): Cyclic dependency between module std.experimental.allocator and std.parallelism
std.experimental.allocator* ->
std.algorithm.comparison ->
std.parallelism* ->
std.file ->
std.experimental.checkedint ->
std.experimental.allocator.common ->
std.experimental.allocator*
…it was set before getter was called at least once
std/experimental/allocator/package.d
Outdated
| // in order to ensure that we use the `processAllocator` setter before the getter | ||
| @system unittest | ||
| { | ||
| import std.experimental.allocator.mallocator: Mallocator; |
There was a problem hiding this comment.
spacing between :
From the CI:
std/experimental/allocator/package.d:233: import std.experimental.allocator.mallocator: Mallocator;
| auto newAlloc = sharedAllocatorObject(Mallocator.instance); | ||
| processAllocator = newAlloc; | ||
| assert(processAllocator is newAlloc); | ||
| processAllocator = sharedAllocatorObject(GCAllocator.instance); |
There was a problem hiding this comment.
Maybe not implicitly assume that it was the GCAllocator before and just reset it to the old value?
There was a problem hiding this comment.
This would require the setter to return the previous allocator, which it currently doesn't. In order to change the API to do this, we'd need to guard with a lock. We can regard this as a best effort.
There was a problem hiding this comment.
This would require the setter to return the previous allocator, which it currently doesn't.
That would also be a potential landmine since for basic types the result of (a = b) is the post-modification value of a, not the pre-modification value of a.
6 participants
…it was set before getter was called at least once