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

Add aliases in AbstractMemory for size_t and ssize_t. #958

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Kerilk
Copy link
Contributor

@Kerilk Kerilk commented May 26, 2022

Hello,

This is a feature I end up re-implementing in most of my gems, so I thought maybe it warranted to be here.
This implements read_size_t and read_ssize_t and their counterparts in AbstractMemory.

The proposal is rather straightforward, but some aspects may warrant discussion:

  • I kept the _t suffix as I didn't want any confusion between get_size and size methods,
  • I do this at runtime, but as sizeof(size_t) is a compile time constant it shouldn't matter,
  • This only support size of 32 and 64 bits, but I think I noticed this assumption in several places in the code,
  • I refactored the ALIAS macro, I avoided using C99 argument macro with no token for maximum portability, but it may feel clunky.

This would also close #537

Thanks

@eregon
Copy link
Collaborator

eregon commented May 27, 2022

Just passing by, this would need to be tested.

@Kerilk
Copy link
Contributor Author

Kerilk commented May 27, 2022

@eregon I though so as well, but I couldn't find where the other aliases are tested. Maybe they are and I just missed it.

@eregon
Copy link
Collaborator

eregon commented May 27, 2022

struct_spec.rb has related tests

@Kerilk
Copy link
Contributor Author

Kerilk commented May 27, 2022

@eregon I ended up adding tests to pointer_spec.rb as write_array_of_type and read_array_of_type were somewhat tested there. I hope it is fine.
Truffle ruby seems to be getting those methods from https://github.com/oracle/truffleruby/blob/master/src/main/ruby/truffleruby/core/truffle/ffi/pointer_access.rb so these would have to be added there as well.

@eregon
Copy link
Collaborator

eregon commented May 27, 2022

I just realized, actually this is already possible with the read/write/get/put methods:

irb(main):008:0> ptr = FFI::MemoryPointer.new(100)
=> #<FFI::MemoryPointer address=0x0000000001deb6c0 size=100>
irb(main):009:0> ptr.write(:size_t, 42)
=> nil
irb(main):010:0> ptr.read(:size_t)
=> 42

So I think we should not add new methods, because it's already easily possible via a general way.
Adding new methods means more time to define them (during loading), increased footprint and overhead for all 3 backends to support it (C ext, JRuby, TruffleRuby).

I'm sorry I didn't think of that earlier, would have spared you some effort.

@Kerilk
Copy link
Contributor Author

Kerilk commented May 27, 2022

For the sake of the argument: size_t is a native C type that is encountered very frequently, and is platform specific, so you need a bit of logic to define those aliases in your code. Especially if you want to read/write array of those as the reader/writer does not exist:
p.read_array_of_type(:size_t, XXXXX, 3)

So I agree with you in general but I think size_t should be covered the same as int or long or pointer or float or double.

Edit: Maybe ssize_t could be replaced by ptrdiff_t, but I prefer the symmetry here.

@eregon
Copy link
Collaborator

eregon commented May 27, 2022

I think it'd be good to add something like ptr.read_array(type, length) (and write_array).
The existing read_array_of_type feels clunky due to specifying both the type and the method to read/write (redundant arguments).

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.

How does one read a value of :size_t from a MemoryPointer?
2 participants