Skip to content

Comments

Add GCHeapMallocator#8555

Closed
pbackus wants to merge 1 commit intodlang:masterfrom
pbackus:add-gc-heap-mallocator
Closed

Add GCHeapMallocator#8555
pbackus wants to merge 1 commit intodlang:masterfrom
pbackus:add-gc-heap-mallocator

Conversation

@pbackus
Copy link
Contributor

@pbackus pbackus commented Sep 3, 2022

In preparation for the removal of GCAllocator.deallocate and
GCAllocator.reallocate, make that functionality available via a new
allocator with a different name.

Users of GCAllocator that wish to continue using manual @system memory
management instead of automatic @safe memory management can switch to
GCHeapMallocator.

For the rationale behind these changes, see PR #8554 and issue 23318.


CC @CyberShadow @schveiguy

@atilaneves this adds a new symbol

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @pbackus! 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

Auto-close Bugzilla Severity Description
23318 enhancement GCAllocator should not implement deallocate

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 + phobos#8555"

@pbackus
Copy link
Contributor Author

pbackus commented Sep 3, 2022

core.exception.AssertError@std/logger/core.d(1905): foo

Looks like a flaky test. Force pushing to retry...

@burner
Copy link
Member

burner commented Sep 4, 2022

#8557

@pbackus pbackus force-pushed the add-gc-heap-mallocator branch from e7d3730 to 38b9177 Compare September 4, 2022 23:13
@pbackus
Copy link
Contributor Author

pbackus commented Sep 4, 2022

Rebased to pick up #8557

@schveiguy
Copy link
Member

Can this be an alias/wrapper underneath? Hard to review this when most likely it's simply a copy-paste of existing code.

@pbackus pbackus force-pushed the add-gc-heap-mallocator branch from 38b9177 to d7cccf1 Compare September 5, 2022 19:49
@pbackus
Copy link
Contributor Author

pbackus commented Sep 5, 2022

@schveiguy Made it a wrapper.

@pbackus pbackus force-pushed the add-gc-heap-mallocator branch from d7cccf1 to 3039f40 Compare September 5, 2022 19:59
}

/**
Manual allocator that allocates from the D runtime's garbage-collected heap.
Copy link
Member

Choose a reason for hiding this comment

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

I suggest describing the distinctions in more detail, i.e. explicitly mentioning that this allocator uses GC.free, and that the other does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the docs for GCHeapMallocator here; will do the same for GCAllocator in the other PR.

Comment on lines +221 to +227
pure nothrow @trusted void[] allocate(size_t bytes) shared const
{
return impl.allocate(bytes);
}

Copy link
Member

Choose a reason for hiding this comment

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

Would alias this work here, instead of explicit forwards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alias this to a private field doesn't work when used from outside the module, and even if it did, I don't think the implicit conversion would be desirable.

@pbackus pbackus force-pushed the add-gc-heap-mallocator branch from 3039f40 to 81f1942 Compare September 5, 2022 20:26
In preparation for the removal of GCAllocator.deallocate and
GCAllocator.reallocate, make that functionality available via a new
allocator with a different name.

Users of GCAllocator that wish to continue using manual @System memory
management instead of automatic @safe memory management can switch to
GCHeapMallocator.

For the rationale behind these changes, see PR dlang#8554 and issue 23318.
@pbackus pbackus force-pushed the add-gc-heap-mallocator branch from 81f1942 to 7aa3c3a Compare September 5, 2022 20:33
@atilaneves
Copy link
Contributor

Please see my comments on the other PR.

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.

6 participants