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: Fortran/PR19303 PATCH: Runtime selection of record markers forunformatted sequential io


Janne Blomqvist wrote:
> the following patch implements the first steps towards the ability to
> choose between various record marker schemes for unformatted
> sequential I/O at runtime.

Very cool!

> - Support for HP format (see
> http://gcc.gnu.org/ml/fortran/2004-12/msg00142.html ) is not
> implemented completely nor correctly, since I can't figure out how it
> is supposed to work at all without very poor performance (see comments
> in source).


> - Although the code is structured so that each unit may use a
> different record marker format, currently the only way to change the
> format is via a magic command line switch "-frecm", whose value can be
> either "-frecm=g77" or "-frecm=hp" (the default format cannot be set
> with this switch and is the same format as gfortran previously
> used). It would be relatively straightforward to add an intrinsic to
> change the default record marker type for new units, or maybe
> implement it as an extra option to the OPEN statement.

Maybe introducing an environment variable for this makes sense?  (See
runtime/environ.c).  Or making this a compile-time option instead of a
runtime-option.

> Patch and Changelog attached.

I'll make a few comments on the coding style, so you can fix this, before our
reviewer finds time.

> Index: Makefile.in
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/libgfortran/Makefile.in,v
> retrieving revision 1.30
> diff -u -p -r1.30 Makefile.in
> --- Makefile.in	23 Jan 2005 17:00:58 -0000	1.30
> +++ Makefile.in	18 Feb 2005 22:52:42 -0000
> Index: aclocal.m4
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/libgfortran/aclocal.m4,v
> retrieving revision 1.6
> diff -u -p -r1.6 aclocal.m4
> --- aclocal.m4	2 Dec 2004 11:04:22 -0000	1.6
> +++ aclocal.m4	18 Feb 2005 22:52:42 -0000

Please don't include generated files in the diff.

> Index: fmain.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/libgfortran/fmain.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 fmain.c
> --- fmain.c	13 May 2004 06:40:59 -0000	1.2
> +++ fmain.c	18 Feb 2005 22:52:42 -0000
> @@ -13,6 +13,10 @@ main (int argc, char *argv[])
>    /* Set up the runtime environment.  */
>    set_args (argc, argv);
>  
> +  /* Handle setting default record marker (need cmdline arguments,
> +     __attribute__((constructor)) cannot be used here). */
                                                         ^^^^
Here and everywhere else: comments are supposed to end in "punctuation, two
blanks, */", i.e. "used here).  */"

> Index: libgfortran.h
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/libgfortran/libgfortran.h,v
> retrieving revision 1.23
> diff -u -p -r1.23 libgfortran.h
> --- libgfortran.h	1 Feb 2005 09:06:22 -0000	1.23
> +++ libgfortran.h	18 Feb 2005 22:52:43 -0000
> @@ -469,6 +469,11 @@ internal_proto(init_units);
>  extern void close_units (void);
>  internal_proto(close_units);
>  
> +/* record_marker.c */
> +
> +extern int set_default_record_marker (int, char **);
> +export_proto(set_default_record_marker);
> +

Shouldn't that be an library-internal function?

> /* Copyright (C) 2002, 2003, 2004, 2005 Free Software Foundation, Inc.
>    Contributed by Janne Blomqvist and Andy Vaught

How?  I'm just curious.

> /* Set the record marker for a unit. */
> 
> int
> set_record_marker (gfc_unit *unit, int recmt)
> {
>   switch (recmt)
>     {
>     case RECM_G77:
>       unit->read_record_marker = &record_marker_g77_r;
>       unit->write_record_marker = &record_marker_g77_w;
>       unit->us_read = &us_read_g77;
>       unit->us_write = &us_write_g77;
>       break;
>     case RECM_GFORTRAN:
>       unit->read_record_marker = &record_marker_gfortran_r;
>       unit->write_record_marker = &record_marker_gfortran_w;
>       unit->us_read = &us_read_gfortran;
>       unit->us_write = &us_write_gfortran;
>       break;
>     case RECM_HP:
>       unit->read_record_marker = &record_marker_hp_r;
>       unit->write_record_marker = &record_marker_hp_w;
>       unit->us_read = &us_read_gfortran;
>       unit->us_write = &us_write_gfortran;
>       break;
>     default:
>       return FAILURE;
>     }

I'm wondering if it's not simpler to only add a 'marker_type' field to the
unit, and then write generic {read|write}_record_marker, us_read and us_write
routines which decide which routine to call by looking at the type of marker
set for the unit.  I don't think this would perform worse, as the functions
won't have to be called through a function pointer in exchange for the
additional switch + tailcall.

Good work,
- Tobi


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