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

[ludo@gnu.org (Ludovic Courtès)]

Hi Neil,

Sorry for the delay, I was off-line last week.

"Neil Jerram" <neiljerram@googlemail.com> writes:

> Agreed.  Yesterday I was worried about possible confusion from
> bypassing a port's read buffer at a time when the read buffer has some
> data in it.  But in fact this is not a problem, because
> scm_fill_input() should be (and in practice is) only called when the
> port's read buffer is empty.

Right.  Putting an assertion in there to make sure can't hurt.

> Aesthetics are always tricky.  I accept your point here, but against
> that we have to count
>
> - the cost of adding a new method to libguile's port definition
>
> - the benefit of (some) existing fill_input code being able to work
> with caller-supplied buffers automatically.  (I say "some", because
> some fill_input implementations may make assumptions about the read
> buffer size and so not automatically take advantage of a larger
> caller-supplied buffer.  But at least the important fport case will
> work automatically.)

Yeah, right.

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?

> Based on all this, I'd like to update my suggestion so that
>
> - scm_fill_input_buf always uses the caller's buffer
>
> - the remaining scm_fill_input_buf logic is inlined into scm_c_read
> (since that is the only place that uses scm_fill_input_buf, and
> inlining it permits some further simplifications).

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.

> +  /* Now we will call scm_fill_input repeatedly until we have read the
> +     requested number of bytes.  (Note that a single scm_fill_input
> +     call does not guarantee to fill the whole of the port's read
> +     buffer.)  For these calls, since we already have a buffer here to
> +     read into, we bypass the port's own read buffer (if it has one),
> +     by saving it off and modifying the port structure to point to our
> +     own buffer. */
> +  saved_buf = pt->read_buf;
> +  saved_buf_size = pt->read_buf_size;
> +  pt->read_pos = pt->read_buf = pt->read_end = buffer;
> +  pt->read_buf_size = size;

`fill_input' methods can raise an exception, as is the case with soft
parts.  Thus, this code should be protected by a `dynwind' that
restores the port's buffer and size.

> +      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).

Other than that, I'm OK with your patch.

Thanks,
Ludovic.