fix issue 16398 - experimental allocators, add aligned reallocation for Posix #5857
Conversation
ghost
requested review from
andralex,
wilzbach and
PetarKirov
as code owners
|
Thanks for your pull request, @bbasile! 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 references
|
ghost
changed the title
| auto p = alignedAllocate(s, a); | ||
| if (!p.ptr) | ||
| { | ||
| deallocate(b); |
There was a problem hiding this comment.
We don't want to deallocate the original memory here - just signal failure and let the client decide.
| } | ||
| else | ||
| { | ||
| if (b.ptr) |
There was a problem hiding this comment.
no need for this test - it works itself out
| { | ||
| import std.algorithm.comparison : min; | ||
| const upTo = min(s, b.length); | ||
| p[0..upTo] = b[0..upTo]; |
| @@ -68,16 +68,14 @@ struct Mallocator | |||
| } | |||
|
|
|||
| /// | |||
| @nogc nothrow | |||
| @system unittest | |||
| @nogc @system nothrow unittest | |||
| { | |||
| auto buffer = Mallocator.instance.allocate(1024 * 1024 * 4); | |||
There was a problem hiding this comment.
I wonder why these lines appear green? Is it because they weren't covered and now they are?
There was a problem hiding this comment.
I have not the CodeCov plugin enabled and i see it green too, so it's the standard diff color.
| On Posix, uses $(D alignedAllocate) and copies data around because there is | ||
| no realloc for aligned memory. On Windows, calls | ||
| $(HTTP msdn.microsoft.com/en-US/library/y69db7sx(v=vs.80).aspx, | ||
| On Posix this function is a helper that allocates and copies data around |
There was a problem hiding this comment.
The previous Posix-related explanation was more informative, even though it was incorrect :o).
| no realloc for aligned memory. On Windows, calls | ||
| $(HTTP msdn.microsoft.com/en-US/library/y69db7sx(v=vs.80).aspx, | ||
| On Posix this function is a helper that allocates and copies data around | ||
| because there is no realloc for aligned memory. |
There was a problem hiding this comment.
please put "realloc" in backquotes
| $(HTTP msdn.microsoft.com/en-US/library/y69db7sx(v=vs.80).aspx, | ||
| On Posix this function is a helper that allocates and copies data around | ||
| because there is no realloc for aligned memory. | ||
| On Windows, calls $(HTTP msdn.microsoft.com/en-US/library/y69db7sx(v=vs.80).aspx, |
There was a problem hiding this comment.
Use HTTPS so we're with the cool kids. Also the link has changed, now it's https://msdn.microsoft.com/en-us/library/y69db7sx.aspx
You may want to even try something like $(LUCKY __aligned_realloc site:microsoft.com, text text text)
| On Posix, uses $(D alignedAllocate) and copies data around because there is | ||
| no realloc for aligned memory. On Windows, calls | ||
| $(HTTP msdn.microsoft.com/en-US/library/y69db7sx(v=vs.80).aspx, | ||
| On Posix this function is non system call that allocates and copies old data |
There was a problem hiding this comment.
"this function is non system call" does not parse, I'll fix the text myself
| { | ||
| return false; | ||
| } | ||
| else |
|
hmmm what did you do Andrei ? It was green. |
|
there was a whitespace |
|
thanks, green again. |
The rationale for adding the primitive is that, like explained on bugzilla, the Windows version made for the Digital Mars standard C library already included a "pseudo" alignedReallocate that actually just allocates a new block, copies the old one and frees it.