Re: [patch, libgfortran] PR47694 [4.3/4.4/4.5/4.6 Regression] Fortran read from named pipe fails

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

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.


-- 
Janne Blomqvist