[libfortran,3/3] Update file position lazily

On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
> Also, I think I've found a small standards conformance bug. From F2008
> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
> without changing its position." and "If the file has been repositioned
> since the connection, the scalar-default-char-variable
> is assigned a processor-dependent value, which shall not be REWIND
> unless the file is positioned at its initial
> point and shall not be APPEND unless the file is positioned so that its
> endfile record is the next record or at its
> terminal point if it has no endfile record.
> "
> 
> If my understanding of the above is correct, returning ASIS is
> incorrent unless the position is unchanged since the OPEN statement.
> Currently we return ASIS by default if it's neither REWIND nor APPEND.
> So the patch changes the implementation to return the
> processor-dependent value UNSPECIFIED in this case.
> 
If my reading is correct, returning ASIS is as valid as returning UNSPECIFIED 
("processor-dependent"). I have a preference for UNSPECIFIED and see your 
patch as OK, but shouldn't it be avoided if it breaks backwards compatibility?

I'm also afraid of testsuite changes of the following kind.
Was there no reason for the "-std=legacy"?

On Sat, Oct 29, 2011 at 01:48, Mikael Morin <mikael.morin@sfr.fr> wrote:
> On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
>> Also, I think I've found a small standards conformance bug. From F2008
>> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
>> without changing its position." and "If the file has been repositioned
>> since the connection, the scalar-default-char-variable
>> is assigned a processor-dependent value, which shall not be REWIND
>> unless the file is positioned at its initial
>> point and shall not be APPEND unless the file is positioned so that its
>> endfile record is the next record or at its
>> terminal point if it has no endfile record.
>> "
>>
>> If my understanding of the above is correct, returning ASIS is
>> incorrent unless the position is unchanged since the OPEN statement.
>> Currently we return ASIS by default if it's neither REWIND nor APPEND.
>> So the patch changes the implementation to return the
>> processor-dependent value UNSPECIFIED in this case.
>>
> If my reading is correct, returning ASIS is as valid as returning UNSPECIFIED
> ("processor-dependent"). I have a preference for UNSPECIFIED and see your
> patch as OK, but shouldn't it be avoided if it breaks backwards compatibility?

My thinking was that the first sentence I quoted would prohibit ASIS
even though it's not explicitly forbidden in the second quoted
sentence. Fixing the implementation would thus be correcting a
standards-conformance bug.

FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
case could be made for using the same. Comments?

> I'm also afraid of testsuite changes of the following kind.
> Was there no reason for the "-std=legacy"?
>
> diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90
> b/gcc/testsuite/gfortran.dg/inquire_5.f90
> index fe107a1..064f96d 100644
> --- a/gcc/testsuite/gfortran.dg/inquire_5.f90
> +++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
> @@ -1,11 +1,10 @@
>  ! { dg-do run { target fd_truncate } }
> -! { dg-options "-std=legacy" }
>  !

I changed the declaration of "chr" from "character*20" to
"character(len=20)" which made std=legacy unnecessary. As the testcase
doesn't test any legacy functionality per se, I though this change
would slightly simplify it. See also

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40881

which mass-added std=legacy to a number of testcases (including this
one) as a result of some frontend warnings changes.

Thanks for the reviews!

On Saturday 29 October 2011 10:09:07 Janne Blomqvist wrote:
> On Sat, Oct 29, 2011 at 01:48, Mikael Morin <mikael.morin@sfr.fr> wrote:
> > On Tuesday 18 October 2011 17:11:24 Janne Blomqvist wrote:
> >> Also, I think I've found a small standards conformance bug. From F2008
> >> (N1830) 9.10.2.23 (page 256): "... ASIS if the connection was opened
> >> without changing its position." and "If the file has been repositioned
> >> since the connection, the scalar-default-char-variable
> >> is assigned a processor-dependent value, which shall not be REWIND
> >> unless the file is positioned at its initial
> >> point and shall not be APPEND unless the file is positioned so that its
> >> endfile record is the next record or at its
> >> terminal point if it has no endfile record.
> >> "
> >> 
> >> If my understanding of the above is correct, returning ASIS is
> >> incorrent unless the position is unchanged since the OPEN statement.
> >> Currently we return ASIS by default if it's neither REWIND nor APPEND.
> >> So the patch changes the implementation to return the
> >> processor-dependent value UNSPECIFIED in this case.
> > 
> > If my reading is correct, returning ASIS is as valid as returning
> > UNSPECIFIED ("processor-dependent"). I have a preference for UNSPECIFIED
> > and see your patch as OK, but shouldn't it be avoided if it breaks
> > backwards compatibility?
> 
> My thinking was that the first sentence I quoted would prohibit ASIS
> even though it's not explicitly forbidden in the second quoted
> sentence. Fixing the implementation would thus be correcting a
> standards-conformance bug.
Well, the first sentence does impose the value to be ASIS in case the position 
hasn't changed, but it does not impose the value not to be ASIS in case the 
position has changed.
On the other hand, let's think about the use cases: if we are returning ASIS 
even if the position has changed, we can't use that value reliably to tell 
that the position hasn't changed.
Thus, I think your patch is OK with the following changes.

> 
> FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
> case could be made for using the same. Comments?
Let's go for UNDEFINED then.

> 
> > I'm also afraid of testsuite changes of the following kind.
> > Was there no reason for the "-std=legacy"?
> > 
> > diff --git a/gcc/testsuite/gfortran.dg/inquire_5.f90
> > b/gcc/testsuite/gfortran.dg/inquire_5.f90
> > index fe107a1..064f96d 100644
> > --- a/gcc/testsuite/gfortran.dg/inquire_5.f90
> > +++ b/gcc/testsuite/gfortran.dg/inquire_5.f90
> > @@ -1,11 +1,10 @@
> >  ! { dg-do run { target fd_truncate } }
> > -! { dg-options "-std=legacy" }
> >  !
> 
> I changed the declaration of "chr" from "character*20" to
> "character(len=20)" which made std=legacy unnecessary. As the testcase
> doesn't test any legacy functionality per se, I though this change
> would slightly simplify it. See also
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40881
> 
> which mass-added std=legacy to a number of testcases (including this
> one) as a result of some frontend warnings changes.
OK for that part.

>
> @@ -31,7 +30,8 @@
>        write(7,*)'this is another record'
>        backspace(7)
>        inquire(7,position=chr)
> -      if (chr.NE.'ASIS') CALL ABORT
> +      if (chr.eq.'ASIS' .or. chr .eq. 'REWIND' &
> +           .or. chr .eq. 'APPEND') CALL ABORT
I think it's better to keep the more restrictive:
         if (chr.NE.'UNDEFINED') CALL ABORT
Just in case we have a memory leak some day, which makes us return some junk 
here.

Please give the other some time to comment on our discussion before 
committing.
Thanks for the patch.

Mikael

On Saturday 29 October 2011 14:43:22 Mikael Morin wrote:
> > FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
> > case could be made for using the same. Comments?
> 
> Let's go for UNDEFINED then.
On second thought, UNSPECIFIED is better as UNDEFINED is for another case.

On Sat, Oct 29, 2011 at 18:35, Mikael Morin <mikael.morin@sfr.fr> wrote:
> On Saturday 29 October 2011 14:43:22 Mikael Morin wrote:
>> > FWIW, it seems ifort 12.0 uses "UNDEFINED" in this case; I suppose a
>> > case could be made for using the same. Comments?
>>
>> Let's go for UNDEFINED then.
> On second thought, UNSPECIFIED is better as UNDEFINED is for another case.

Hmm, indeed, on second thought I agree as well.

Further comparisons:

pathf95 3.2.99: UNKNOWN

pgf95 11.2-0: REWIND (which is clearly wrong, also inquire_5.f90
failed earlier because trying to get the position of a direct access
file returned ASIS while the standard requires UNDEFINED in this
case).