[PATCH] Prefer to use rb_io_descriptor in my_fileno

[PATCH] Prefer to use rb_io_descriptor in my_fileno

[Samuel Williams]

---
 ext/raindrops/extconf.rb  | 1 +
 ext/raindrops/my_fileno.h | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/ext/raindrops/extconf.rb b/ext/raindrops/extconf.rb
index 792e509..1733703 100644
--- a/ext/raindrops/extconf.rb
+++ b/ext/raindrops/extconf.rb
@@ -4,6 +4,7 @@
 dir_config('atomic_ops')
 have_func('mmap', 'sys/mman.h') or abort 'mmap() not found'
 have_func('munmap', 'sys/mman.h') or abort 'munmap() not found'
+have_func('rb_io_descriptor')
 
 $CPPFLAGS += " -D_GNU_SOURCE "
 have_func('mremap', 'sys/mman.h')
diff --git a/ext/raindrops/my_fileno.h b/ext/raindrops/my_fileno.h
index 4c8ffba..00c5d29 100644
--- a/ext/raindrops/my_fileno.h
+++ b/ext/raindrops/my_fileno.h
@@ -3,6 +3,12 @@
 
 static int my_fileno(VALUE io)
 {
+#ifdef HAVE_RB_IO_DESCRIPTOR
+	if (TYPE(io) != T_FILE)
+		io = rb_convert_type(io, T_FILE, "IO", "to_io");
+
+	return rb_io_descriptor(io);
+#else
 	rb_io_t *fptr;
 
 	if (TYPE(io) != T_FILE)
@@ -12,4 +18,5 @@ static int my_fileno(VALUE io)
 	if (fptr->fd < 0)
 		rb_raise(rb_eIOError, "closed stream");
 	return fptr->fd;
+#endif
 }
-- 
2.39.2 (Apple Git-143)

Re: [PATCH] Prefer to use rb_io_descriptor in my_fileno

[Eric Wong]

Samuel Williams <samuel.williams@oriontransfer.co.nz> wrote:
> ---

Where's the commit message body?

At minimum, non-trivial commits must have an explanation of why
a change is necessary.  This is important to reviewers who don't
follow Ruby core development closely.


Even one sentence for the "why" is fine as long as it can be
meaningful to offline reviewers.  References to online sources are
fine (full URLs, not #NNN); but it needs to be accompanied by a
summary for offline users.  See `git log' for examples.

In the case of this patch; I would also like to know which Ruby
implementation(s) and version(s) it appears in; and evidence
backing up that it's a supported public API we can depend on
going forward.


On a side note, describing "what" you're doing file-by-file
is totally uninteresting when it's obvious from reading
the patch.  IOW, old ruby.git and GNU ChangeLog conventions
are a waste of time+effort for users with `git log -p'.


Please ensure HTML is off and keep raindrops-public Cc-ed in responses.

Thanks.

Re: [PATCH] Prefer to use rb_io_descriptor in my_fileno

[Samuel Williams]

Eric, it’s part of CRuby’s public headers since 3.1 was released.

Introduced in 4b8903421828cb9d4de139180563ae8d8f04e1ab

Hope that helps.

Warm regards,
Samuel

> On 9/06/2023, at 7:16 PM, Eric Wong <e@80x24.org> wrote:
> 
> In the case of this patch; I would also like to know which Ruby
> implementation(s) and version(s) it appears in; and evidence
> backing up that it's a supported public API we can depend on
> going forward.

Re: [PATCH] Prefer to use rb_io_descriptor in my_fileno

[Jean Boussier]

>  the "why"

I think the missing piece here is that this change is necessary for compatibility with the current ruby master:

https://bugs.ruby-lang.org/issues/19057#note-17

kgio seem to have the same code and fail in the same way.

Re: [PATCH] Prefer to use rb_io_descriptor in my_fileno

[Eric Wong]

Jean Boussier <jean.boussier@gmail.com> wrote:
> >  the "why"
> 
> I think the missing piece here is that this change is necessary for compatibility with the current ruby master:
> 
> https://bugs.ruby-lang.org/issues/19057#note-17

Thanks, that's an important note omitted from both original patches.

I've pushed out Samuel's patch as commit
b3212417cc3e7cc44aa9e1ffe89b0d62ef3fdab5 to raindrops.git
with your comment

> kgio seem to have the same code and fail in the same way.

+Cc kgio-public@yhbt.net

I'll take documented patches for kgio, otherwise I'll try to deal with it
before Ruby 3.3 (or when Debian stable ships Ruby 3.3 :P).


I've also posted some raindrops cleanups and modernizations
which should reduce stack frames and binary size for 3.1+
(untested since I don't have space/time for multiple Ruby
installs atm):
https://yhbt.net/raindrops-public/20230611213328.379546-1-bofh@yhbt.net/