Skip to content
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

ae.utils.appender: check for Allocator.deallocate #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pbackus
Copy link

@pbackus pbackus commented Sep 3, 2022

Needed to unblock dlang/phobos#8554

@CyberShadow
Copy link
Owner

Maybe I don't understand something, but would it not be useful for data structure implementations to enforce the availability of allocator primitives? E.g. some data structures may be grow-only, and therefore truly not need the deallocate method, and some may allocate/deallocate memory and therefore actually need for the allocator to support deallocation?

As it is, this change will make passing an allocator that truly can't deallocate silently leak memory.

I will follow up in the Phobos PR as well.

@pbackus
Copy link
Author

pbackus commented Sep 3, 2022

If a data structure absolutely requires deterministic deallocation, then yes, of course it can require that its allocator implement deallocate (and it should probably also check the return value, to make sure it's working).

In general, though, if a user goes out of their way to create an allocator that leaks memory, and instantiates a container with that allocator, it probably should not be the container's job to second-guess the user's intentions. Sometimes programmers leak memory on purpose (see: DMD).

@CyberShadow
Copy link
Owner

In general, though, if a user goes out of their way to create an allocator that leaks memory, and instantiates a container with that allocator, it probably should not be the container's job to second-guess the user's intentions.

I think a more correct and elegant approach to address that would be a shim layer as described here: dlang/phobos#8554 (comment)

@CyberShadow
Copy link
Owner

BTW on the topic of shim layers, it should be easier to write them:
https://github.com/CyberShadow/btdu/blob/c5dc72d82ee98f0efdd37ef3830fead00bf44a63/source/btdu/alloc.d#L60-L61

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.

2 participants