This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]