Fix Issue 16824 - std.experimental.allocator.dispose leaks memory #4982
Fix Issue 16824 - std.experimental.allocator.dispose leaks memory #4982JackStouffer merged 4 commits intodlang:masterfrom
Conversation
…ays of more than 1 dimension
|
RazvanN7
force-pushed
the
Issue_16824
branch
from
1d1fd9c to
afb43d4
Compare
…ays of more than 1 dimension
RazvanN7
force-pushed
the
Issue_16824
branch
from
afb43d4 to
936a802
Compare
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Watch the style bot (aka CircleCi) ;-)
std/experimental/allocator/package.d
Outdated
| struct TestAllocator | ||
| { | ||
| import std.experimental.allocator.common: platformAlignment; | ||
| import std.experimental.allocator.mallocator: Mallocator; |
There was a problem hiding this comment.
style nit: whitespace between the colon : (see also the failed CircleCi or run make -f posix.mak style)
|
|
||
| assert(_allocations.canFind!pred); | ||
|
|
||
| _allocations = _allocations.remove!pred; |
There was a problem hiding this comment.
FYI: there is countUntil which allows to the search only once, but of course it doesn't matter for this simple test.
std/experimental/allocator/package.d
Outdated
| bool deallocate(void[] bytes) | ||
| { | ||
| import std.algorithm: remove, canFind; | ||
| import std.conv: text; |
std/experimental/allocator/package.d
Outdated
|
|
||
| foreach (ref ints1d; ints2d) | ||
| foreach(ref ints0d; ints1d) | ||
| ints0d = allocator.makeArray!(int)(4); |
There was a problem hiding this comment.
parenthesis aren't needed here and how about adding this to foreach loop before?
| ints0d = allocator.makeArray!(int)(4); | ||
|
|
||
| allocator.dispose(ints2d); | ||
| } |
There was a problem hiding this comment.
How about doing the same test with something that has a destructor and thus verifying that the destructor is called for all elements? e.g.
static instances = 0;
struct Foo
{
~this()
{
instances = 0;
}
// allocate
assert(instances == 100);
// dispose
assert(instances == 0);
|
Should we add a deallocation for pointers? |
|
Hmmm... it's debatable. I'll comment in https://issues.dlang.org/show_bug.cgi?id=16824 |
|
Ping @RazvanN7 |
|
@JackStouffer Maybe this PR and associated bug is invalid? As you can see the discussion at [1] and the fact that there is already a function called makeNdarray which seems to do the exact same thing [2]. [1] https://issues.dlang.org/show_bug.cgi?id=16824 |
|
@9il do you think this is needed? |
|
I'm not sure |
| /** | ||
| Allocates a multidimensional array of elements of type T. | ||
|
|
||
| Params: |
There was a problem hiding this comment.
we don't use capital names for values, just use n
There was a problem hiding this comment.
I thought that this is common for statically defined arrays [1][2]
[1] https://dlang.org/library/std/experimental/ndslice/selection/iota_slice.html
[2] https://github.com/dlang/phobos/blob/master/std/experimental/ndslice/slice.d#L834
There was a problem hiding this comment.
Thx for the good examples, though no exception is justified there. We should probably slowly fix those as the code gets touched instead of introducing more instances.
cc @wilzbach: all values and functions are likeThis, all types are LikeThis, aliases that may be either should generally be likeThis, modules and labels should be like_this.
|
|
||
| Returns: | ||
| An N-dimensional array with individual elements of type T. | ||
| */ |
| return makeArray!T(alloc, lengths[0]); | ||
| } | ||
| else | ||
| { |
| import std.experimental.allocator.mallocator : Mallocator; | ||
|
|
||
| size_t[3] dimArray = [2, 3, 6]; | ||
| auto mArray = Mallocator.instance.makeMultidimensionalArray!(dimArray.length, int)(dimArray); |
There was a problem hiding this comment.
guess it's nice to also deallocate here with scope(exit)
| array = the multidimensional array that is to be deallocated | ||
| */ | ||
| void disposeMultidimensionalArray(Allocator, T)(auto ref Allocator alloc, T[] array) | ||
| { |
|
@andralex Hope we're good now. Thanks for reviewing! |
|
Merging due to Andrei's approval |
|
Auto-merge toggled on |
6 participants
The dispose function now checks recursively if there are multidimensional arrays allocated and deallocates them accordingly