On Sat, Feb 19, 2011 at 20:27, Jerry DeLisle<jvdelisle@frontier.com> wrote:
On 02/19/2011 09:15 AM, Janne Blomqvist wrote:
if (oldpos + *len> oldact)
{
+ /* See if we have a pending EOR in here. */
+ if (*len> oldact)
This second conditional makes no sense. What matters is if the current
position in the buffer + len goes past the active bytes, which is
exactly the previous conditional. Thus, does the patch work if you
remove that conditional and check for EOR unconditionally if oldpos +
*len> oldact?
I thought it made no sense either. But without it, fmt_t_2.f90 fails. It is
the only regression I found when testing this patch. I determined that
condition by experiment. I looked at the values of oldpos, oldact, *len for
various test cases to see what was unique about that one in particular. I
then tried different combinations of conditions until it worked for the
piped case and this case and no regressions. I suppose we could study that
case more explicitly, but it is unique. Maybe a bug elsewhere?
I'm very suspicious about this; I see no logical reason how it could
make sense. Perhaps a bug elsewhere, yes.
Secondly, you check for "\n" or "\r", but I think you need to check
that a "\r" is followed by a "\n" before you can say that you're at
EOR.
It does not matter because either way the result is we read what is in the
buffer and sf_read looks for the \r followed by \n. All we are doing here is
avoiding an sread and I wanted to address the case where there is a \r
without a \n. Of course I just realized I can artificially test this on my
machine here and I will do so.
Good point.
I wonder, would it perhaps be cleaner to create a new function, say
something like
static inline uchar *
fbuf_getptr (gfc_unit * u)
{
return (uchar*) u->fbuf->buf + u->fbuf->pos;
}
use this in read_sf() to get a pointer to eventually return, then loop
forwards one character at a time using fbuf_getc() checking for
EOR/EOF and other conditions/errors along the way.
What do you think?
This alternate approach could work, but I think the numerous calls to
fbuf_getc even if inlined will be less efficient then the approach I have
which is a fairly tight loop.
Do you want to test and compare? It will take me a little while to modify
and test it. Also, how exactly should we test for performance? cat bigfile
tmpfifo? or just read a big file? and time it.
I suspect the fbuf_getc() approach might be faster, as in that case we
don't need to scan through the string twice (once in fbuf_read(), once
in read_sf()), but in any case I'm not that concerned about the
performance. I'd just like something that is straightforward to
understand and which makes sense, so we reduce the possibility of
further regressions.