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

Return the module handle value in Fiddle::Handle#to_i and add Fiddle::Handle#to_ptr #87

Merged
merged 2 commits into from Jul 14, 2021

Conversation

mrkn
Copy link
Member

@mrkn mrkn commented Jul 13, 2021

Fiddle::Handle#to_i currently returns the address of the value of the module handle, that is &fiddle_handle->ptr.
I guess it is more useful than now that it returns the value of the module handle directly.

@mrkn mrkn requested a review from kou July 13, 2021 13:56
@mrkn
Copy link
Member Author

mrkn commented Jul 13, 2021

I get the another proposal from @unak to introduce the new method Fiddle::Handle#value for this purpose.

@mrkn
Copy link
Member Author

mrkn commented Jul 13, 2021

Other approach: Introducing Fiddle::Handle#value or Fiddle::Handle#pointer and let them return a Fiddle::Pointer instead of an integer.

@kou
Copy link
Member

kou commented Jul 14, 2021

This is a backward incompatible change but the current Handle#to_i is useless. So nobody will get a trouble.
I'm OK with this change.

I also like Fiddle::Handle#pointer approach but how about #to_pointer instead of #pointer?

@mrkn
Copy link
Member Author

mrkn commented Jul 14, 2021

OK. I will add to_ptr instead of to_pointer because Fiddle::Pointer.[] examine to_ptr.

@kou kou merged commit 170111a into master Jul 14, 2021
@kou kou deleted the to_i_return_handle branch July 14, 2021 02:26
@kou
Copy link
Member

kou commented Jul 14, 2021

Thanks!

@kou kou changed the title Return the module handle value in Fiddle::Handle#to_i Return the module handle value in Fiddle::Handle#to_i and add Fiddle::Handle#to_ptr Jul 14, 2021
matzbot pushed a commit to ruby/ruby that referenced this pull request Jul 14, 2021
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.

None yet

2 participants