Make resolveInternalPointer @safe#5783
Conversation
edi33416
requested review from
PetarKirov,
andralex and
wilzbach
as code owners
|
Thanks for your pull request, @edi33416! 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. |
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
Cool. One question is whether we should make this pure as well.
| bool deallocate(void[] b) | ||
| { | ||
| if (!b.ptr) return; | ||
| if (!b.ptr) return true; |
There was a problem hiding this comment.
how in the world did this pass until now?
There was a problem hiding this comment.
The only reasonable explanation is that the member function or its parent template were never instantiated.
There was a problem hiding this comment.
This code doesn't get hit because of the __EOF__ here.
|
@andralex I missed that. I think we should make this |
| assert(r == Ternary.yes && &p[0] == &buffer[0] && p.length >= buffer.length); | ||
| assert(GCAllocator.instance.resolveInternalPointer(null, p) == Ternary.no); | ||
| void *badPtr = (() @trusted => cast(void*)(0xdeadbeef))(); | ||
| assert(GCAllocator.instance.resolveInternalPointer(badPtr, p) == Ternary.no); |
There was a problem hiding this comment.
@edi33416 Circle CI fails with:
std/experimental/allocator/gc_allocator.d(180:56)[warn]: Argument 2 is named 'p', but this is the name of parameter 1
So the course of action is to use a slightly more informative name for the local variables. I would suggest renaming:
void[] p->void[] resultTernary r->Ternary found
There was a problem hiding this comment.
Or alternatively, void[] p -> void[] res and Ternary r -> Ternary ok if you prefer to be more terse.
edi33416
force-pushed
the
safe_alloc_resolv_int_ptr
branch
from
d621a46 to
d7d51a6
Compare
|
Should I remove this Please note that by doing so, we will make |
|
Yes, ThreadLocal is good but let's make that a separate PR with a changelog entry associated. Thanks! |
edi33416
force-pushed
the
safe_alloc_resolv_int_ptr
branch
3 times, most recently
from
cf2307a to
88f806e
Compare
|
@edi33416 rebase |
edi33416
force-pushed
the
safe_alloc_resolv_int_ptr
branch
from
88f806e to
c1efb05
Compare
|
cc @CyberShadow, DAutoTest fails with: Will force pull this. |
This usually means that code.dlang.org is down. |
|
@CyberShadow thx, can you translate to a more informative error message on the scripting side? Thx! |
|
I feel really bad about doing that. @s-ludwig Could you please fix Dub to provide a better error message instead? |
The 2.077.0 release contains DUB 1.6.0 which has fallback mirrors and better error messages, so the easiest path would be to bump the D compiler version |
6 participants
For stand-alone allocators, such as
GCAllocator,resolveInternalPointeris a@safefunction.Allocators that are building on top of such allocators should infer the safety from their parents.
This PR is a subset of #5330, as this approach will provide us with better granularity. More smaller PRs to come :)