-
Notifications
You must be signed in to change notification settings - Fork 37
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
Structured memory deallocation using blocks #38
Conversation
chrisseaton
commented
Jun 3, 2020
- Easy to use best-practice
- Helps people write correct code for the most common cases
- Protect against leaks (free if there's an exception, or even if someone uses a trace point to get hold of the object)
- Protect against deallocation latency (free as soon as you're done)
- Protect against non-locality (free while it's still in cache from your using it)
ext/fiddle/pointer.c
Outdated
return obj; | ||
if (rb_block_given_p()) { | ||
if (f == NULL) { | ||
rb_raise(rb_eFiddleError, "a free function must be supplied to Fiddle::Pointer.malloc when it is called with a block"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use rb_eArgError
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
ext/fiddle/pointer.c
Outdated
@@ -261,7 +271,14 @@ rb_fiddle_ptr_s_malloc(int argc, VALUE argv[], VALUE klass) | |||
obj = rb_fiddle_ptr_malloc(s,f); | |||
if (wrap) RPTR_DATA(obj)->wrap[1] = wrap; | |||
|
|||
return obj; | |||
if (rb_block_given_p()) { | |||
if (f == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (f == NULL) { | |
if (!f) { |
Because we don't use == NULL
in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/fiddle/test_pointer.rb
Outdated
|
||
def test_malloc_block_no_free | ||
assert_raise Fiddle::DLError do | ||
Pointer.malloc(10) { |ptr| raise } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove needless raise
? raise
in assert_raise
may confuse us.
Pointer.malloc(10) { |ptr| raise } | |
Pointer.malloc(10) { |ptr| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Sorry I didn't use 'commit suggestion' I hadn't seen that before in GitHub and didn't realise it was an option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
No problem.