| Message ID | 48e1b516-f5ff-f437-c734-f8e6633dd467@codethink.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | PR fortran/89103 - Allow blank format items in format strings | expand |
On 10/06/2019 15:10, Mark Eggleston wrote: > > On 10/06/2019 15:07, Thomas Koenig wrote: >> Am 10.06.19 um 15:33 schrieb Mark Eggleston: >>> This patch is for a customer that has a huge codebase and that is >>> the only reason for its existence. >> >> I didn't know gfortran as a whole has customers as such :-) >> >>> The error message has not changed. Making it more informative, >>> indicating an option that will allow this non-standard usage, is a >>> bad idea as it could result in its spread. >> >> OK, I understand that. So scrap the idea of pointing towards the >> option. >> >> However, making it more informative _without_ pointing towards that >> option is still a good idea (such as "missing item in format list"). > That's a much better idea. I'll implement that and when it's ready > I'll update the patch. Patch updated and attached. ChangeLogs: gcc/fortran Jim MacArthur <jim.macarthur@codethink.co.uk> Mark Eggleston <mark.eggleston@codethink.com> PR fortran/89103 * gfortran.texi: Add -fdec-blank-format-item * invoke.texi: Add to section on Commas in FORMAT specifications. * io.c (check_format): Add new string missing_item. * io.c (check_format): At FMT_RPAREN goto finished if -fdec-blank-format-item otherwise set error string to missing_item. * lang.opt: Add new option. * options.c (set_dec_flags): Add SET_BITFLAG for flag_dec_format_defaults. Jim MacArthur <jim.macarthur@codethink.co.uk> Mark Eggleston <mark.eggleston@codethink.com> gcc/testsuite PR fortran/89103 * gfortran.dg/dec_format_empty_item_1.f: New test. * gfortran.dg/dec_format_empty_item_2.f: New test. * gfortran.dg/dec_format_empty_item_3.f: New test. If OK, please can someone commit this. >> >> If we want to allow legacy extensions to clutter our code, getting >> a more informative error message is something we can get in return, >> at least. >> >> Regards >> >> Thomas >>
On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote: > Jim MacArthur <jim.macarthur@codethink.co.uk> > Mark Eggleston <mark.eggleston@codethink.com> Two spaces before < instead of one. This is not a patch review, just comments: > This has to be written in a slightly verbose manner because GCC 7 > defaults to building with -Werror=implicit-fallthrough which prevents > us from just falling through to the default: case. That is not true, one can fall through just fine, just there needs to be a comment or attribute or builtin that says so. > --- a/gcc/fortran/io.c > +++ b/gcc/fortran/io.c > @@ -598,6 +598,7 @@ check_format (bool is_input) > { > const char *posint_required = _("Positive width required"); > const char *nonneg_required = _("Nonnegative width required"); > + const char *missing_item = _("Missing item"); > const char *unexpected_element = _("Unexpected element %qc in format " > "string at %L"); > const char *unexpected_end = _("Unexpected end of format string"); > @@ -756,6 +757,16 @@ format_item_1: > error = unexpected_end; > goto syntax; > > + case FMT_RPAREN: > + /* Oracle allows a blank format item. */ > + if (flag_dec_blank_format_item) > + goto finished; > + else > + { > + error = missing_item; > + goto syntax; > + } So, if you want to fall thru, just do: case FMT_RPAREN: if (flag_dec_blank_format_item) goto finished; /* FALLTHRU */ > + > default: > error = unexpected_element; > goto syntax; and that is it. Not sure I'd mention the Oracle fortran compiler there, either it is common to other DEC based compilers too (DEC fortran, ifort, ...) and then the comment makes no sense, or it might not be best to call the flag -fdec-whatever. Furthermore, even if you want to have a _("Missing item"), you should write it as error = _("Missing item");, not the way it is written, as that way it is inefficient at compile time. The rest is just a general comment on the preexisting code. Using a bunch of const char *whatever = _("..."); at the start of function is undesirable, it means any time this function is called, even in the likely case there is no error, all those strings need to be translated. It would be better to e.g. replace all _("...") in that function with G_("...") (i.e. mark for translation, but don't translate), and only when actually using that translate: if (error == unexpected_element) gfc_error (error, error_element, &format_locus); else gfc_error ("%s in format string at %L", error, &format_locus); The first case is translated already by gfc_error, the second one would need _(error) instead of error (but is generally wrong anyway, because you are constructing a diagnostics from two pieces which might not be ok for translations. So, likely you want to append " in format string at %L" to all the error string literals inside of G_("...") and just pass error as first argument to gfc_error. Jakub
On Tue, Jun 11, 2019 at 11:50:40AM +0200, Jakub Jelinek wrote: > On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote: > > Jim MacArthur <jim.macarthur@codethink.co.uk> > > Mark Eggleston <mark.eggleston@codethink.com> > > Two spaces before < instead of one. > > This is not a patch review, just comments: Mark, do you plan to address any of Jakub's comments. Do note, I just 'OK' Jakub's patch that uses G_() forms for the strings. Also, do you have plans to contribute additional patches (either for -fdec* extensions or preferrably to help with bug fixes and new features)? It may be advantageous for you to get a commit bit.
On 12/06/2019 19:11, Steve Kargl wrote: > On Tue, Jun 11, 2019 at 11:50:40AM +0200, Jakub Jelinek wrote: >> On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote: >>> Jim MacArthur <jim.macarthur@codethink.co.uk> >>> Mark Eggleston <mark.eggleston@codethink.com> >> Two spaces before < instead of one. >> >> This is not a patch review, just comments: > Mark, do you plan to address any of Jakub's comments. > Do note, I just 'OK' Jakub's patch that uses G_() > forms for the strings. Now that Jakubs's patch has been committed, please find attached an updated patch and updated change logs: gcc/fortran Jim MacArthur <jim.macarthur@codethink.co.uk> Mark Eggleston <mark.eggleston@codethink.com> PR fortran/89103 * gfortran.texi: Add -fdec-blank-format-item * invoke.texi: Add option to list of options. * invoke.texi: Add to section on Commas in FORMAT specifications. * io.c (check_format): At FMT_RPAREN goto finished if -fdec-blank-format-item otherwise set error string. * lang.opt: Add new option. * options.c (set_dec_flags): Add SET_BITFLAG for flag_dec_format_defaults. gcc/testsuite Jim MacArthur <jim.macarthur@codethink.co.uk> Mark Eggleston <mark.eggleston@codethink.com> PR fortran/89103 * gfortran.dg/dec_format_empty_item_1.f: New test. * gfortran.dg/dec_format_empty_item_2.f: New test. * gfortran.dg/dec_format_empty_item_3.f: New test. as before... Please can someone commit this as do not have commit rights. > > Also, do you have plans to contribute additional > patches (either for -fdec* extensions or preferrably > to help with bug fixes and new features)? It may be > advantageous for you to get a commit bit. Yes, I do intend to contribute additional patches, mostly -fdec- patches, there are also some patches unrelated to -fdec* extensions.
I will see if I can get this one. On 6/17/19 6:37 AM, Mark Eggleston wrote: > > On 12/06/2019 19:11, Steve Kargl wrote: >> On Tue, Jun 11, 2019 at 11:50:40AM +0200, Jakub Jelinek wrote: >>> On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote: >>>> Jim MacArthur <jim.macarthur@codethink.co.uk> >>>> Mark Eggleston <mark.eggleston@codethink.com> >>> Two spaces before < instead of one. >>> >>> This is not a patch review, just comments: >> Mark, do you plan to address any of Jakub's comments. >> Do note, I just 'OK' Jakub's patch that uses G_() >> forms for the strings. > > Now that Jakubs's patch has been committed, please find attached an updated > patch and updated change logs: > > gcc/fortran > > Jim MacArthur <jim.macarthur@codethink.co.uk> > Mark Eggleston <mark.eggleston@codethink.com> > > PR fortran/89103 > * gfortran.texi: Add -fdec-blank-format-item > * invoke.texi: Add option to list of options. > * invoke.texi: Add to section on Commas in FORMAT specifications. > * io.c (check_format): At FMT_RPAREN goto finished if > -fdec-blank-format-item otherwise set error string. > * lang.opt: Add new option. > * options.c (set_dec_flags): Add SET_BITFLAG for > flag_dec_format_defaults. > > gcc/testsuite > > Jim MacArthur <jim.macarthur@codethink.co.uk> > Mark Eggleston <mark.eggleston@codethink.com> > > PR fortran/89103 > * gfortran.dg/dec_format_empty_item_1.f: New test. > * gfortran.dg/dec_format_empty_item_2.f: New test. > * gfortran.dg/dec_format_empty_item_3.f: New test. > > as before... Please can someone commit this as do not have commit rights. > >> >> Also, do you have plans to contribute additional >> patches (either for -fdec* extensions or preferrably >> to help with bug fixes and new features)? It may be >> advantageous for you to get a commit bit. > Yes, I do intend to contribute additional patches, mostly -fdec- patches, there > are also some patches unrelated to -fdec* extensions. > >
From f04143dd423d2eda2ce206e42ae914374b273f82 Mon Sep 17 00:00:00 2001 From: Jim MacArthur <jim.macarthur@codethink.co.uk> Date: Thu, 4 Feb 2016 16:59:41 +0000 Subject: [PATCH 1/5] Allow blank format items in format strings This has to be written in a slightly verbose manner because GCC 7 defaults to building with -Werror=implicit-fallthrough which prevents us from just falling through to the default: case. Test written by: Francisco Redondo Marchena <francisco.marchena@codethink.co.uk> Use -fdec-blank-format-item to enable. Also enabled by -fdec. --- gcc/fortran/gfortran.texi | 7 ++++++- gcc/fortran/invoke.texi | 13 +++++++++---- gcc/fortran/io.c | 10 ++++++++++ gcc/fortran/lang.opt | 4 ++++ gcc/fortran/options.c | 1 + gcc/testsuite/gfortran.dg/dec_format_empty_item_1.f | 19 +++++++++++++++++++ gcc/testsuite/gfortran.dg/dec_format_empty_item_2.f | 19 +++++++++++++++++++ gcc/testsuite/gfortran.dg/dec_format_empty_item_3.f | 19 +++++++++++++++++++ 8 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/dec_format_empty_item_1.f create mode 100644 gcc/testsuite/gfortran.dg/dec_format_empty_item_2.f create mode 100644 gcc/testsuite/gfortran.dg/dec_format_empty_item_3.f diff --git a/gcc/fortran/gfortran.texi b/gcc/fortran/gfortran.texi index 0e6c57142cd..2d53202dd71 100644 --- a/gcc/fortran/gfortran.texi +++ b/gcc/fortran/gfortran.texi @@ -1761,11 +1761,16 @@ When omitted, the count is implicitly assumed to be one. To support legacy codes, GNU Fortran allows the comma separator to be omitted immediately before and after character string edit -descriptors in @code{FORMAT} statements. +descriptors in @code{FORMAT} statements. A comma with no following format +decriptor is permited if the @option{-fdec-blank-format-item} is given on +the command line. This is considered non-conforming code and is +discouraged. @smallexample PRINT 10, 2, 3 10 FORMAT ('FOO='I1' BAR='I2) + print 20, 5, 6 +20 FORMAT (I3, I3,) @end smallexample diff --git a/gcc/fortran/invoke.texi b/gcc/fortran/invoke.texi index 63fce66a593..80804993522 100644 --- a/gcc/fortran/invoke.texi +++ b/gcc/fortran/invoke.texi @@ -119,10 +119,10 @@ by type. Explanations are in the following sections. @gccoptlist{-fall-intrinsics -fbackslash -fcray-pointer -fd-lines-as-code @gol -fd-lines-as-comments -fdec -fdec-structure -fdec-intrinsic-ints @gol -fdec-static -fdec-math -fdec-include -fdec-format-defaults @gol --fdefault-double-8 -fdefault-integer-8 -fdefault-real-8 -fdefault-real-10 @gol --fdefault-real-16 -fdollar-ok -ffixed-line-length-@var{n} @gol --ffixed-line-length-none -fpad-source -ffree-form @gol --ffree-line-length-@var{n} -ffree-line-length-none @gol +-fdec-blank-format-item -fdefault-double-8 -fdefault-integer-8 @gol +-fdefault-real-8 -fdefault-real-10 -fdefault-real-16 -fdollar-ok @gol +-ffixed-line-length-@var{n} -ffixed-line-length-none -fpad-source @gol +-ffree-form -ffree-line-length-@var{n} -ffree-line-length-none @gol -fimplicit-none -finteger-4-integer-8 -fmax-identifier-length @gol -fmodule-private -ffixed-form -fno-range-check -fopenacc -fopenmp @gol -freal-4-real-10 -freal-4-real-16 -freal-4-real-8 -freal-8-real-10 @gol @@ -288,6 +288,11 @@ be on a single line and can use line continuations. Enable format specifiers F, G and I to be used without width specifiers, default widths will be used instead. +@item -fdec-blank-format-item +@opindex @code{fdec-blank-format-item} +Enable a blank format item at the end of a format specification i.e. nothing +following the final comma. + @item -fdollar-ok @opindex @code{fdollar-ok} @cindex @code{$} diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c index 80a0d7402d2..4d0de65be1a 100644 --- a/gcc/fortran/io.c +++ b/gcc/fortran/io.c @@ -756,6 +756,16 @@ format_item_1: error = unexpected_end; goto syntax; + case FMT_RPAREN: + /* Oracle allows a blank format item. */ + if (flag_dec_blank_format_item) + goto finished; + else + { + error = unexpected_element; + goto syntax; + } + default: error = unexpected_element; goto syntax; diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt index 26e82601b62..cc2eae238b9 100644 --- a/gcc/fortran/lang.opt +++ b/gcc/fortran/lang.opt @@ -440,6 +440,10 @@ fdec Fortran Var(flag_dec) Enable all DEC language extensions. +fdec-blank-format-item +Fortran Var(flag_dec_blank_format_item) +Enable the use of blank format items in format strings. + fdec-include Fortran Var(flag_dec_include) Enable legacy parsing of INCLUDE as statement. diff --git a/gcc/fortran/options.c b/gcc/fortran/options.c index 9ba48dc8439..e32aa441822 100644 --- a/gcc/fortran/options.c +++ b/gcc/fortran/options.c @@ -75,6 +75,7 @@ set_dec_flags (int value) SET_BITFLAG (flag_dec_math, value, value); SET_BITFLAG (flag_dec_include, value, value); SET_BITFLAG (flag_dec_format_defaults, value, value); + SET_BITFLAG (flag_dec_blank_format_item, value, value); } /* Finalize DEC flags. */ diff --git a/gcc/testsuite/gfortran.dg/dec_format_empty_item_1.f b/gcc/testsuite/gfortran.dg/dec_format_empty_item_1.f new file mode 100644 index 00000000000..ed27c18944b --- /dev/null +++ b/gcc/testsuite/gfortran.dg/dec_format_empty_item_1.f @@ -0,0 +1,19 @@ +! { dg-do run } +! { dg-options "-fdec" } +! +! Test blank/empty format items in format string +! +! Test case contributed by Jim MacArthur <jim.macarthur@codethink.co.uk> +! Modified by Mark Eggleston <mark.eggleston@codethink.com> +! + PROGRAM blank_format_items + INTEGER A/0/ + + OPEN(1, status="scratch") + WRITE(1, 10) 100 + REWIND(1) + READ(1, 10) A + IF (a.NE.100) STOP 1 + PRINT 10, A +10 FORMAT( I5,) + END diff --git a/gcc/testsuite/gfortran.dg/dec_format_empty_item_2.f b/gcc/testsuite/gfortran.dg/dec_format_empty_item_2.f new file mode 100644 index 00000000000..2793cb16225 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/dec_format_empty_item_2.f @@ -0,0 +1,19 @@ +! { dg-do run } +! { dg-options "-fdec-blank-format-item" } +! +! Test blank/empty format items in format string +! +! Test case contributed by Jim MacArthur <jim.macarthur@codethink.co.uk> +! Modified by Mark Eggleston <mark.eggleston@codethink.com> +! + PROGRAM blank_format_items + INTEGER A/0/ + + OPEN(1, status="scratch") + WRITE(1, 10) 100 + REWIND(1) + READ(1, 10) A + IF (a.NE.100) STOP 1 + PRINT 10, A +10 FORMAT( I5,) + END diff --git a/gcc/testsuite/gfortran.dg/dec_format_empty_item_3.f b/gcc/testsuite/gfortran.dg/dec_format_empty_item_3.f new file mode 100644 index 00000000000..499db922876 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/dec_format_empty_item_3.f @@ -0,0 +1,19 @@ +! { dg-do compile } +! { dg-options "-fdec -fno-dec-blank-format-item" } +! +! Test blank/empty format items in format string +! +! Test case contributed by Jim MacArthur <jim.macarthur@codethink.co.uk> +! Modified by Mark Eggleston <mark.eggleston@codethink.com> +! + PROGRAM blank_format_items + INTEGER A/0/ + + OPEN(1, status="scratch") + WRITE(1, 10) 100 ! { dg-error "FORMAT label 10 at \\(1\\) not defined" } + REWIND(1) + READ(1, 10) A ! { dg-error "FORMAT label 10 at \\(1\\) not defined" } + IF (a.NE.100) STOP 1 + PRINT 10, A ! { dg-error "FORMAT label 10 at \\(1\\) not defined" } +10 FORMAT( I5,) ! { dg-error "Unexpected element" } + END -- 2.11.0