Skip to content

Templated _d_arrayshrinkfit implementation #21173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

yashjha123
Copy link

Draft PR: Possible duplicate of #21152 This PR completely templatizes the _d_arrayshrinkfit hook and adds more unittests that cover good edge cases. This PR may be deleted if there is a conflict, and is therefore a draft.

This PR introduces a template version of the _d_arrayshrinkfit DRuntime Hook, often used as a call with assumesafeappend.

Changes Made

  1. Templated Hook: Implement a new template version of the hook in DRuntime
  2. Change the lowering in the compiler (capacity.d) to use the templated version
  3. Created unit-tests to check for any issues (tests all the edge cases I can think of)
  4. Removed the old hook from DRuntime

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @yashjha123! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#21173"

Copy link
Member

@teodutu teodutu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start. Apply my suggestions and keep it up!

Comment on lines +161 to +180

@safe unittest
{
struct S
{
float f = 1.0;
}

int[] arr;
_d_arraysetlengthTImpl!(typeof(arr))._d_arraysetlengthT(arr, 16);
assert(arr.length == 16);
foreach (int i; arr)
assert(i == int.init);

shared S[] arr2;
_d_arraysetlengthTImpl!(typeof(arr2))._d_arraysetlengthT(arr2, 16);
assert(arr2.length == 16);
foreach (s; arr2)
assert(s == S.init);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you add this test? It's about _d_arraysetlengthT.

ti = `TypeInfo` of array type
arr = array to shrink. Its `.length` is element length, not byte length, despite `void` type
*/
void _d_arrayshrinkfit(T)(void[] arr) nothrow
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
void _d_arrayshrinkfit(T)(void[] arr) nothrow
void _d_arrayshrinkfit(T)(T[] arr) @trusted

The compiler infers pure, @safe and nothrow for templates. But hooks are almost never safe, so mark them as @trusted.

// invalid situation, or no change.
return;

static if (is(T == struct) && hasElaborateDestructor!T)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static if (is(T == struct) && hasElaborateDestructor!T)
static if (hasElaborateDestructor!T)

Don't limit this to structs. Classes can have destructors too.

{
try
{
finalize_array!T(arr.ptr + reqsize, cursize - reqsize);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can inline the function call here

for (auto curP = p + size - tsize; curP >= p; curP -= tsize)
{
// call destructor
destroy(*cast(T*)curP);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
destroy(*cast(T*)curP);
(cast(T*) curP).__xdtor();

This calls the destructor directly since you already know the type has one.

Comment on lines +213 to +246
// Shrinking a shared array
@safe unittest
{
shared(int)[] arr = new shared int[100];
arr.length = 10;
_d_arrayshrinkfit!(shared int)(cast(void[])arr);
assert(arr.length == 10);
}


// Shrink array with no elements (ptr is null)
@safe unittest
{
int[] arr;
assert(arr.ptr is null);
_d_arrayshrinkfit!int(arr); // should be a no-op
assert(arr.length == 0);
}


// Shrinking an array of class references (destroyable via GC)
@safe unittest
{
class C { int x = 5; }
C[] arr = new C[10];
foreach (ref c; arr)
c = new C();

arr.length = 3;
_d_arrayshrinkfit!C(arr);
assert(arr.length == 3);
foreach (c; arr)
assert(c !is null && c.x == 5);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The essence of _d_arrayshrinkfit is that any appends made after calling it will not reallocate. Testing if the elements are the same is fine, but the way to test this fully is to check if the array's .ptr is the same before and after calling the hook.

Comment on lines +18 to +19
bool gc_shrinkArrayUsed(void[] slice, size_t existingUsed, bool atomic) nothrow;
void[] gc_getArrayUsed(void *ptr, bool atomic) nothrow;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
bool gc_shrinkArrayUsed(void[] slice, size_t existingUsed, bool atomic) nothrow;
void[] gc_getArrayUsed(void *ptr, bool atomic) nothrow;
bool gc_shrinkArrayUsed(void[] slice, size_t existingUsed, bool atomic) pure nothrow;
void[] gc_getArrayUsed(void *ptr, bool atomic) pure nothrow;

You'll most likely have to lie about purity too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants