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

Improve documentation on how to correctly free memory and free memory in tests #33

Merged
merged 1 commit into from
May 18, 2020

Conversation

chrisseaton
Copy link
Contributor

The documentation for pointers and structs doesn't include as much helpful information as it could about who is responsible for freeing memory and how to do it safely. I've added some specific examples for guidance.

I've then also applied this in the test suite, which I think was not freeing memory in many tests.

Or am I misunderstanding how freeing pointers and structs works in Fiddle?

If I'm understanding it all correctly, I'd then like to follow this up with some additions to the API to make it even easier to do the right thing, such as

Fiddle::Pointer.malloc(size) do |pointer|
   ...
end
# memory freed when coming out of scope
pointer = Fiddle::Pointer.malloc(size)
pointer.free_now
# calls Fiddle.free
pointer = Fiddle::Pointer.malloc(size, free_routine)
pointer.free_now
# calls Fiddle.free and also de-registers the `freefunc`, for manual freeing but also GC as a backup

@@ -73,7 +73,12 @@ module CStructBuilder
#
# MyStruct = Fiddle::CStructBuilder.create(Fiddle::CUnion, types, members)
#
# obj = MyStruct.allocate
# obj = MyStruct.malloc
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think allocate was ever correct?

@tenderlove
Copy link
Member

Adding the block API makes sense to me

@tenderlove tenderlove merged commit e59cfd7 into ruby:master May 18, 2020
@chrisseaton chrisseaton deleted the free branch May 18, 2020 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants