kou commented on code in PR #50234:
URL: https://github.com/apache/arrow/pull/50234#discussion_r3455442906
##########
ruby/red-arrow/ext/arrow/memory-view.cpp:
##########
@@ -222,6 +222,8 @@ namespace red_arrow {
auto view_ = reinterpret_cast<memory_view *>(view);
view_->obj = obj;
view_->private_data = new PrivateData();
+ view_->item_desc.components = NULL;
+ view_->item_desc.length = 0;
Review Comment:
Could you move them just before `view_->ndim = ...` to align the order in
`red_arrow::memory_view`/`rb_memory_view_t`?
https://github.com/ruby/ruby/blob/e9e7f9b6b9937641b0d7f1c04c483c1cb1e8664a/include/ruby/memory_view.h#L118-L132
Or how about using `memset()`?
```cpp
auto view_ = reinterpret_cast<memory_view *>(view);
memset(view_, 0, sizeof(memory_view));
view_->obj = obj;
...
```
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -29,6 +29,48 @@ def little_endian?
[1].pack("s") == [1].pack("s<")
end
+ def rb_memory_view_functions
+ libruby = Fiddle.dlopen(nil)
+
+ [
+ Fiddle::Function.new(
+ libruby["rb_memory_view_get"],
+ [
+ Fiddle::TYPE_UINTPTR_T,
+ Fiddle::TYPE_VOIDP,
+ Fiddle::TYPE_INT,
+ ],
+ Fiddle::TYPE_INT
Review Comment:
```suggestion
Fiddle::TYPE_BOOL
```
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -431,4 +473,14 @@ def little_endian?
])
end
end
+
+ test("Int32Array with uninitialized rb_memory_view_t") do
+ array = Arrow::Int32Array.new([1, 2, 3, 4, 5])
+ assert_memory_view_release(array)
+ end
+
+ test("Buffer with uninitialized rb_memory_view_t") do
+ buffer = Arrow::Buffer.new("hello")
+ assert_memory_view_release(buffer)
+ end
Review Comment:
Could you add a sub test case and move them to there?
```suggestion
sub_test_case("uninitialized rb_memory_view_t") do
def setup
@rb_memory_view_get = ...
@rb_memory_view_release = ...
end
def assert_release(...)
...
end
test("Int32Array") do
array = Arrow::Int32Array.new([1, 2, 3, 4, 5])
assert_release(array)
end
test("Buffer") do
buffer = Arrow::Buffer.new("hello")
assert_release(buffer)
end
end
```
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -29,6 +29,48 @@ def little_endian?
[1].pack("s") == [1].pack("s<")
end
+ def rb_memory_view_functions
+ libruby = Fiddle.dlopen(nil)
+
+ [
+ Fiddle::Function.new(
+ libruby["rb_memory_view_get"],
+ [
+ Fiddle::TYPE_UINTPTR_T,
+ Fiddle::TYPE_VOIDP,
+ Fiddle::TYPE_INT,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ Fiddle::Function.new(
+ libruby["rb_memory_view_release"],
+ [
+ Fiddle::TYPE_VOIDP,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ ]
+ end
+
+ def assert_memory_view_release(target)
+ rb_memory_view_get, rb_memory_view_release = rb_memory_view_functions
+
+ # Fill the buffer with non-zero garbage to mimic an
+ # uninitialized rb_memory_view_t.
+ view = Fiddle::Pointer.malloc(256)
+ 256.times do |i|
+ view[i] = 0xAA
+ end
+
+ assert_equal(
+ 1,
+ rb_memory_view_get.call(Fiddle.dlwrap(target), view, 0)
+ )
+ assert_nothing_raised do
+ rb_memory_view_release.call(view)
+ end
Review Comment:
Could you use `assert_equal` instead?
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -29,6 +29,48 @@ def little_endian?
[1].pack("s") == [1].pack("s<")
end
+ def rb_memory_view_functions
+ libruby = Fiddle.dlopen(nil)
+
+ [
+ Fiddle::Function.new(
+ libruby["rb_memory_view_get"],
+ [
+ Fiddle::TYPE_UINTPTR_T,
+ Fiddle::TYPE_VOIDP,
+ Fiddle::TYPE_INT,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ Fiddle::Function.new(
+ libruby["rb_memory_view_release"],
+ [
+ Fiddle::TYPE_VOIDP,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ ]
+ end
+
+ def assert_memory_view_release(target)
+ rb_memory_view_get, rb_memory_view_release = rb_memory_view_functions
+
+ # Fill the buffer with non-zero garbage to mimic an
+ # uninitialized rb_memory_view_t.
+ view = Fiddle::Pointer.malloc(256)
Review Comment:
We should use `sizeof(rb_memory_view_t)` instead of `256` but we don't have
`sizeof(rb_memory_view_t)` and it's too much just for this test...
How about not adding tests for this case entirely?
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -29,6 +29,48 @@ def little_endian?
[1].pack("s") == [1].pack("s<")
end
+ def rb_memory_view_functions
+ libruby = Fiddle.dlopen(nil)
+
+ [
+ Fiddle::Function.new(
+ libruby["rb_memory_view_get"],
+ [
+ Fiddle::TYPE_UINTPTR_T,
+ Fiddle::TYPE_VOIDP,
+ Fiddle::TYPE_INT,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ Fiddle::Function.new(
+ libruby["rb_memory_view_release"],
+ [
+ Fiddle::TYPE_VOIDP,
+ ],
+ Fiddle::TYPE_INT
Review Comment:
```suggestion
Fiddle::TYPE_BOOL
```
##########
ruby/red-arrow/test/test-memory-view.rb:
##########
@@ -29,6 +29,48 @@ def little_endian?
[1].pack("s") == [1].pack("s<")
end
+ def rb_memory_view_functions
+ libruby = Fiddle.dlopen(nil)
+
+ [
+ Fiddle::Function.new(
+ libruby["rb_memory_view_get"],
+ [
+ Fiddle::TYPE_UINTPTR_T,
+ Fiddle::TYPE_VOIDP,
+ Fiddle::TYPE_INT,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ Fiddle::Function.new(
+ libruby["rb_memory_view_release"],
+ [
+ Fiddle::TYPE_VOIDP,
+ ],
+ Fiddle::TYPE_INT
+ ),
+ ]
+ end
+
+ def assert_memory_view_release(target)
+ rb_memory_view_get, rb_memory_view_release = rb_memory_view_functions
+
+ # Fill the buffer with non-zero garbage to mimic an
+ # uninitialized rb_memory_view_t.
+ view = Fiddle::Pointer.malloc(256)
Review Comment:
Could you free allocated memory automatically?
```suggestion
Fiddle::Pointer.malloc(256, Fiddle::RUBY_FREE) do |view|
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
