Make the resolveInternalPointer method abide the IAllocator API#5274
Make the resolveInternalPointer method abide the IAllocator API#5274dlang-bot merged 1 commit intodlang:masterfrom
Conversation
|
Ping @andralex |
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
Looking good, please mind coverage.
| auto p2 = (p1.ptr + p1.length - stateSize!Suffix) | ||
| void[] p1; | ||
| Ternary r = parent.resolveInternalPointer(p, p1); | ||
| if (p1 is null) |
There was a problem hiding this comment.
Here you know that resolveInternalPointer is available in the parent so you need to consult its Ternary result first, not the target p1. So I think the most robust code here is:
if (r != Ternary.yes || p1 is null)
return r;
p1 = p1[stateSize!Prefix .. $];
auto p2 = (p1.ptr + p1.length - stateSize!Suffix)
alignDownTo(Suffix.alignof);
result = p1[0 .. p2 - p1.ptr];
return Ternary.yes;| return p1[0 .. p2 - p1.ptr]; | ||
| result = p1[0 .. p2 - p1.ptr]; | ||
| } | ||
| return r; |
There was a problem hiding this comment.
This function is also not covered, which makes the testers protest. Please add unittests (I know they should have been there before).
| void[] resolveInternalPointer(void* p) | ||
| Ternary resolveInternalPointer(void* p, ref void[] result) | ||
| { | ||
| result = null; |
There was a problem hiding this comment.
The other path ends up assigning result twice, so move this inside the if.
There was a problem hiding this comment.
On second thought I see you have two more paths that use this, so never mind. Looks good.
There was a problem hiding this comment.
On third thought: we are under no obligation to assign anything to result if we return anything but Ternary.yes, so please remove this line entirely!
| */ | ||
| void[] resolveInternalPointer(void*) shared const { return null; } | ||
| Ternary resolveInternalPointer(void*, ref void[]) shared const | ||
| { return Ternary.no; } |
There was a problem hiding this comment.
not covered, please add unittest
| { | ||
| result = impl.resolveInternalPointer(p); | ||
| return Ternary(result.ptr !is null); | ||
| return impl.resolveInternalPointer(p, result); |
There was a problem hiding this comment.
not covered, please add unittest
edi33416
force-pushed
the
allocator_interface
branch
from
21c1dfd to
85000db
Compare
4 participants
Have the resolveInternalPointer method defined in the module's static allocators abide the IAllocator API.