[Patch, fortran] PR28585: Add Fortran 2003 NEW_LINE intrinsic function

[FX Coudert]

Hi Tobias,

Sorry for this late review. Here are comments on your patch (although  
some doc changes are mixed with it):

>> Index: gcc/fortran/intrinsic.c
>> ===================================================================
>> --- gcc/fortran/intrinsic.c	(Revision 117031)
>> +++ gcc/fortran/intrinsic.c	(Arbeitskopie)
>> @@ -1881,6 +1881,10 @@
>>
>>    make_generic ("nearest", GFC_ISYM_NEAREST, GFC_STD_F95);
>>
>> +  add_sym_1 ("new_line", 1, 1, BT_CHARACTER, dc, GFC_STD_F2003,
>> +             gfc_check_new_line, gfc_simplify_new_line, NULL,
>> +             a, BT_CHARACTER, dc, REQUIRED);
>> +
>>    add_sym_2 ("nint", 1, 1, BT_INTEGER, di, GFC_STD_F77,
>>  	     gfc_check_a_ikind, gfc_simplify_nint, gfc_resolve_nint,
>>  	     a, BT_REAL, dr, REQUIRED, kind, BT_INTEGER, di, OPTIONAL);

The first two args to add_sym_1 determine whether the function is  
elemental, and whether it is allowed as an actual argument. Here,  
they should both be 0. Admittedly, they were not used until recently,  
and most functions set them wrong (but I corrected that a few hours  
ago).

>> Index: gcc/fortran/gfortran.texi
>> ===================================================================
>> --- gcc/fortran/gfortran.texi	(Revision 117031)
>> +++ gcc/fortran/gfortran.texi	(Arbeitskopie)
>> @@ -161,7 +161,7 @@
>>  @item
>>  Read a user's program,
>>  stored in a file and containing instructions written
>> -in Fortran 77, Fortran 90 or Fortran 95.
>> +in Fortran 77, Fortran 90, Fortran 95 or Fortran 2003.
>>  This file contains @dfn{source code}.
>>
>>  @item

This really is something different from your patch.  I don't have an  
opinion on that change, but you'd want to submit it separately.

>> @@ -386,7 +387,8 @@
>>  @findex @code{ACCESS}
>>  @cindex file system functions
>>
>> -Not yet implemented in gfortran.
>> +Not yet implemented in gfortran.  Except for querying the  
>> executable state,
>> +the Fortran 95 intrinsic statement @code{INQUIRE} can be used  
>> instead.
>>
>>  @table @asis
>>  @item @emph{Description}:

Same thing as gfortran.texi. We'd be happy to review and apply a  
"misc documentation changes" patch from you, but please keep things  
separate. Also, if you plan to change that, I think it's worthy to  
note that this function is implemented now (I think I remember doing  
it).

>> @@ -396,13 +398,14 @@
>>
>>  @item @emph{Class}:
>>  @item @emph{Syntax}:
>> +Checks whether the file associated with the given file name  
>> exists, is
>> +readable, writeable or executable.
>>  @item @emph{Arguments}:
>>  @item @emph{Return value}:
>>  @item @emph{Example}:
>>  @item @emph{Specific names}:
>>  @item @emph{See also}:
>>  @uref{http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19292, g77  
>> features lacking in gfortran}
>> -
>>  @end table

Why is sentence in the Syntax paragraph?

>> +@item @emph{Arguments}:
>> +@multitable @columnfractions .15 .80
>> +@item @var{C}    @tab The type of the argument shall be a scalar  
>> or array of the
>> +                      type @code{CHARACTER}.
>> +@end multitable

Probably better with "The argument shall be a scalar or array of type  
@code{CHARACTER}." (and not "the type...shall be... of the type  
CHARACTER")

>> Index: gcc/fortran/simplify.c
>> ===================================================================
>> --- gcc/fortran/simplify.c	(Revision 117031)
>> +++ gcc/fortran/simplify.c	(Arbeitskopie)
>> @@ -2543,7 +2532,23 @@
>>    return range_check (result, "NEAREST");
>>  }
>>
>> +gfc_expr *
>> +gfc_simplify_new_line (gfc_expr * e)
>> +{
>> +  gfc_expr *result;
>>
>> +  result = gfc_constant_result (BT_CHARACTER,  
>> gfc_default_character_kind,
>> +                                &e->where);

As the doc said, NEW_LINE returns a scalar character of the same kind  
as its argument. We currently only have one character type, but we  
need to keep the possibility to evolve in the future. You probably want:

    result = gfc_constant_result (BT_CHARACTER, e->ts.kind, &e->where);


So, here's what I suggest: please post an updated patch, taking care  
of the abover comments, and I'll commit it. Could you also add a  
testcase? (something like writing a new_line and a few other  
characters, and reading them back to check them)

I know the whole process may seem heavy and there's a lot to learn,  
but it really helps keeping the quality of the compiler code high.

FX

PS: Since I think it's the first of your patch that I review, welcome  
aboard!

PPS: I'll review your out-of-bounds substring patch today or tomorrow.