Re: [PATCH] Add a `read' method for ports

[Neil Jerram]

Hopefully nearly finishing this off now...!

2008/7/16 Ludovic Courtès <address@hidden>:

>>> Likewise, `scm_c_read ()' might raise an exception, so we may need a
>>> `dynwind' here (my original patch for `uniform-vector-read!' did that).
>>
>> This is covered by the dynwind inside scm_c_read, isn't it?
>
> No, HANDLE must be released here (see my original patch).

I don't think that's right; see "Accessing Arrays from C" in the
manual, especially:

"
   You must take care to always unreserve an array after reserving it,
also in the presence of non-local exits.  To simplify this, reserving
and unreserving work like a dynwind context (*note Dynamic Wind::): a
call to `scm_array_get_handle' can be thought of as beginning a dynwind
context and `scm_array_handle_release' as ending it.  When a non-local
exit happens between these two calls, the array is implicitely
unreserved.

   That is, you need to properly pair reserving and unreserving in your
code, but you don't need to worry about non-local exits.
"

Now, as it happens, the code doesn't actually implement what the
manual says, and in fact scm_array_handle_release() is currently a
no-op!  But I believe the manual's intention is sensible, so it think
I think we should commit this patch as-is for now, and raise a bug to
track the fact that the array/handle API isn't fully implemented.

What do you think?

> My post also contained a `uniform-vector-read!' benchmark, which showed
> a noticeable performance improvement on unbuffered ports.  Could you try
> (and commit) that also?

Yes, I'll do that shortly (before and separately from the libguile
code changes).

> And now for some gratuitous (but friendly) nitpicking...
>
>> +struct port_and_swap_buffer {
>
> Please follow the GNU style.  :-)

Well, they say "The open-brace that starts a struct body can go in
column one if you find it useful to treat that definition as a
defun.".  Is that what you meant?  In this case I don't think it's
important to "treat that definition as a defun" - and also there are
lots of other examples in  libguile where the opening brace is on the
same line...

> A small comment above might be nice.

Right, added:

/* This structure, and the following swap_buffer function, are used
   for temporarily swapping a port's own read buffer, and the buffer
   that the caller of scm_c_read provides. */

>> +  /* Reinstate the port's normal buffer. */
>
> I like adding a space after periods at the end of a sentence.

But there is a space there - so I don't understand!

Regards,
        Neil