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

[Ludovic Courtès]

Good evening!

"Neil Jerram" <address@hidden> writes:

> 2008/6/24 Ludovic Courtès <address@hidden>:

>> These arguments make a lot of sense in 1.8.  In the longer run, though,
>> I'd prefer to see `fill_input' superseded by a `read' method akin to
>> what I proposed.  What do you think?
>
> To be honest, I don't yet see a clear need for it.  If there is no
> drawback to what we have now, and there would be a compatibility cost
> to introducing a new read method, then why do that?

Well, mostly for robustness (passing arguments explicitly) and
aesthetics (removing part of the burden of the internal buffer
management from port implementors, following the "rule of least
surprise").  Also, the port API is currently undocumented, and I
wouldn't feel comfortable documenting `fill_input ()'.

That said, it would indeed have a compatibility cost, which may be a
stronger argument given the current state of affairs.

>> OK.  The issue is that if the caller-supplied buffer is smaller than the
>> port's buffer, we end up reading less data than we'd otherwise do.  I
>> think this scenario should be addressed (by comparing `read_buf_size'
>> and `size'), as was the case in my proposal.
>
> True in theory, but I feel very confident that this will never matter
> in practice, and hence that we don't need to have code for this.

Yeah, the performance difference may not be noticeable.

>>> +      remaining -= scm_c_read (port_or_fd, base + off, remaining);
>>> +      if (remaining % sz != 0)
>>> +        SCM_MISC_ERROR ("unexpected EOF", SCM_EOL);
>>> +      ans -= remaining / sz;
>>
>> 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).

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?


And now for some gratuitous (but friendly) nitpicking...

> +struct port_and_swap_buffer {

Please follow the GNU style.  :-)

A small comment above might be nice.

> +static void
> +swap_buffer (void *data)

Likewise, a comment would help.

> +  /* Reinstate the port's normal buffer. */

I like adding a space after periods at the end of a sentence.

Thanks,
Ludo'.