Make Fiddle::Pointer's MemoryView writable

[KitaitiMakoto]

Hello,

This pull request allows Fiddle::Pointer to export a writable MemoryView.

The presence of the Fiddle::Pointer#[]= method suggests it is possible. If I'm mistaken, please let me know.

Thank you.

[kou]

Could you accept both of SIMPLE (read-only) and WRITABLE?

[KitaitiMakoto]

Let me clarify what you say.

Do you mean adding a test case to accept SIMPLE flag? Or, making the MemoryView readonly when SIMPLE flag is specified?

[kou]

The latter.

[KitaitiMakoto]

Done!

[kou]

How about this style for easy to understand?

diff --git a/ext/fiddle/pointer.c b/ext/fiddle/pointer.c
index 87abc86..d9b3d1c 100644
--- a/ext/fiddle/pointer.c
+++ b/ext/fiddle/pointer.c
@@ -137,10 +137,17 @@ fiddle_ptr_memory_view_available_p(VALUE obj)
 static bool
 fiddle_ptr_get_memory_view(VALUE obj, rb_memory_view_t *view, int flags)
 {
-    if (flags != RUBY_MEMORY_VIEW_SIMPLE) return false;
+    bool read_only = true;
+    if (flags == RUBY_MEMORY_VIEW_SIMPLE) {
+        read_only = true;
+    } else if (flags == RUBY_MEMORY_VIEW_WRITABLE) {
+        read_only = false;
+    } else {
+        return false;
+    }
 
     struct ptr_data *data = fiddle_ptr_check_memory_view(obj);
-    rb_memory_view_init_as_byte_array(view, obj, data->ptr, data->size, true);
+    rb_memory_view_init_as_byte_array(view, obj, data->ptr, data->size, read_only);
 
     return true;
 }

[KitaitiMakoto]

Using a variable named read_only seems good.

However, I think checking whether the flags are supported and determining whether the MemoryView should be read-only are different concerns.

For instance, in the future, this function might accept other flags such as FORMAT and XXX_CONTIGUOUS (which include STRIDES). At that time, the function will check the bits of unsupported flag INDIRECT and return if they're on. It will look like:

if ((flags & RUBY_MEMORY_VIEW_INDIRECT) == RUBY_MEMORY_VIEW_INDIRECT) return false;

On the other hand, only the WRITABLE flag will affect read_only even the flags is combined with other flags:

// Even if `flags` is `RUBY_MEMORY_VIEW_WRITABLE | RUBY_MEMORY_VIEW_ROW_MAJOR`
bool read_only = !(flags & RUBY_MEMORY_VIEW_WRITABLE);

So, how about this:

diff --git a/ext/fiddle/pointer.c b/ext/fiddle/pointer.c
index 2409e61..3bde700 100644
--- a/ext/fiddle/pointer.c
+++ b/ext/fiddle/pointer.c
@@ -137,11 +137,11 @@ fiddle_ptr_memory_view_available_p(VALUE obj)
 static bool
 fiddle_ptr_get_memory_view(VALUE obj, rb_memory_view_t *view, int flags)
 {
-    bool writable_requested = flags & RUBY_MEMORY_VIEW_WRITABLE;
+    bool read_only = !(flags & RUBY_MEMORY_VIEW_WRITABLE);
     if (flags != RUBY_MEMORY_VIEW_SIMPLE && flags != RUBY_MEMORY_VIEW_WRITABLE) return false;
 
     struct ptr_data *data = fiddle_ptr_check_memory_view(obj);
-    rb_memory_view_init_as_byte_array(view, obj, data->ptr, data->size, !writable_requested);
+    rb_memory_view_init_as_byte_array(view, obj, data->ptr, data->size, read_only);
 
     return true;
 }

Is this a YAGNI concern? You are the maintainer of this feature (I guess). What matters most is that it makes sense to you. If you don't agree with me, I will apply your suggestion.

Thank you.

[kou]

My concern is if (flags != RUBY_MEMORY_VIEW_SIMPLE && flags != RUBY_MEMORY_VIEW_WRITABLE) return false; line. if (... != ... && ... != ...) return false (not, not, false) is difficult to understand.

Can we reduce negative parts as much as possible?

[KitaitiMakoto]

That makes sense. I assigned the check result of flags to a variable and pushed it. How about the current patch?

[kou]

OK. Let's use it.

[KitaitiMakoto]

Thank you for the review and merge.