*ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

*ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston
*ping*

On 23/05/2019 09:19, Mark Eggleston wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89103 see comment 4
>
> Please can some one commit the attached patch for me as I do not have
> the privileges to do so.
>
> Change logs:
>
> gcc/fortran
>
>     Jim MacArthur  <[hidden email]>
>     Mark Eggleston  <[hidden email]>
>     PR fortran/89103
>     * gfortran.texi: Add -fdec-blank-format-item
>     * invoke.texi: Add to section on Commas in FORMAT specifications.
>     * io.c (check_format): At FMT_RPAREN goto finished if
>     -fdec-blank-format-item.
>     * lang.opt: Add new option.
>     * options.c (set_dec_flags): Add SET_BITFLAG for
>     flag_dec_format_defaults.
>
> gcc/testsuite
>
>     Jim MacArthur <[hidden email]>
>     Mark Eggleston <[hidden email]>
>
>     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.
>
--
https://www.codethink.co.uk/privacy.html

Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

jerry DeLisle-3
On 6/3/19 4:57 AM, Mark Eggleston wrote:
> *ping*
>
> On 23/05/2019 09:19, Mark Eggleston wrote:
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89103 see comment 4
>>
>> Please can some one commit the attached patch for me as I do not have the
>> privileges to do so.
>>

Mark,

Give me a few days to review the patch, apply it, and test and I will take care
of it unless someone objects. (I have been out of the loop for a bit, silently
present in the lurking dungeons. ;) )

Jerry
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

jerry DeLisle-3
On 6/4/19 5:54 PM, Jerry DeLisle wrote:

> On 6/3/19 4:57 AM, Mark Eggleston wrote:
>> *ping*
>>
>> On 23/05/2019 09:19, Mark Eggleston wrote:
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89103 see comment 4
>>>
>>> Please can some one commit the attached patch for me as I do not have the
>>> privileges to do so.
>>>
>
> Mark,
>
> Give me a few days to review the patch, apply it, and test and I will take care
> of it unless someone objects. (I have been out of the loop for a bit, silently
> present in the lurking dungeons. ;) )
>
> Jerry
>

Committing to svn+ssh://[hidden email]/svn/gcc/trunk ...
        M gcc/testsuite/ChangeLog
        M gcc/testsuite/gfortran.dg/fmt_f_default_field_width_3.f90
        M gcc/testsuite/gfortran.dg/fmt_g_default_field_width_3.f90
Committed r272046

Regards,

Jerry
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston

On 07/06/2019 15:33, Jerry DeLisle wrote:

> On 6/4/19 5:54 PM, Jerry DeLisle wrote:
>> On 6/3/19 4:57 AM, Mark Eggleston wrote:
>>> *ping*
>>>
>>> On 23/05/2019 09:19, Mark Eggleston wrote:
>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89103 see comment 4
>>>>
>>>> Please can some one commit the attached patch for me as I do not
>>>> have the privileges to do so.
>>>>
>>
>> Mark,
>>
>> Give me a few days to review the patch, apply it, and test and I will
>> take care of it unless someone objects. (I have been out of the loop
>> for a bit, silently present in the lurking dungeons. ;) )
>>
>> Jerry
>>
>
> Committing to svn+ssh://[hidden email]/svn/gcc/trunk ...
>     M    gcc/testsuite/ChangeLog
>     M    gcc/testsuite/gfortran.dg/fmt_f_default_field_width_3.f90
>     M    gcc/testsuite/gfortran.dg/fmt_g_default_field_width_3.f90
> Committed r272046
Thanks, that was for PR fortran/89100 to fix its test cases. PR
fortran/89103 also needs doing which I have attached.

gcc/fortran

     Jim MacArthur <[hidden email]>
     Mark Eggleston <[hidden email]>
     PR fortran/89103
     * gfortran.texi: Add -fdec-blank-format-item
     * invoke.texi: Add to section on Commas in FORMAT specifications.
     * io.c (check_format): At FMT_RPAREN goto finished if
     -fdec-blank-format-item.
     * lang.opt: Add new option.
     * options.c (set_dec_flags): Add SET_BITFLAG for
     flag_dec_format_defaults.

gcc/testsuite

     Jim MacArthur <[hidden email]>
     Mark Eggleston <[hidden email]>

     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.

>
> Regards,
>
> Jerry
>
--
https://www.codethink.co.uk/privacy.html


0006-Allow-blank-format-items-in-format-strings.patch (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Thomas Koenig-6
Hi Mark,

> +++ 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;
> + }
> +

If we make an extra effort to support this non-standard code, we
could at least use this to generate a more informative error message
(which could also point to the option that would allow this extension).

Regards

        Thomas
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston

On 10/06/2019 11:30, Thomas Koenig wrote:

> Hi Mark,
>
>> +++ 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;
>> +    }
>> +
>
> If we make an extra effort to support this non-standard code, we
> could at least use this to generate a more informative error message
> (which could also point to the option that would allow this extension).

This patch is for a customer that has a huge codebase and that is the
only reason for its existence. 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. As
is the case with all legacy features and extensions their inclusion
risks pollution of Fortran code with non-standard features which is why
they should only be available using explicit compiler options.

>
> Regards
>
>     Thomas
>
--
https://www.codethink.co.uk/privacy.html

Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Thomas Koenig-6
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").

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
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston

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.
>
> 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
>
--
https://www.codethink.co.uk/privacy.html

Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston

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  <[hidden email]>
     Mark Eggleston  <[hidden email]>

     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 <[hidden email]>
     Mark Eggleston <[hidden email]>

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
>>
--
https://www.codethink.co.uk/privacy.html


0006-Allow-blank-format-items-in-format-strings.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Jakub Jelinek
On Tue, Jun 11, 2019 at 10:30:59AM +0100, Mark Eggleston wrote:
>     Jim MacArthur <[hidden email]>
>     Mark Eggleston <[hidden email]>

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
Reply | Threaded
Open this post in threaded view
|

[PATCH] check_format fixes

Jakub Jelinek
On Tue, Jun 11, 2019 at 11:50:40AM +0200, Jakub Jelinek wrote:

> 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.

It is actually even worse.  On:
      write (10, 100)
      write (10, 200)
100   format (*)
200   format (DT(124:))
      end
gfortran reports:
Error: Left parenthesis required after %<*%> in format string at (1)
and
Error: Right parenthesis expected at %C in format string at (1)
With the following patch which implements the above (but not the -fdec*
addition Mark has been posting), it reports:
Error: Left parenthesis required after ‘*’ in format string at (1)
and
Error: Right parenthesis expected at (1) in format string at (2)
instead.

Tested on x86_64-linux with check-gfortran, ok for trunk if full bootstrap/regtest
passes?

2019-06-11  Jakub Jelinek  <[hidden email]>

        * io.c (check_format): Use G_(...) instead of _(...) for error values,
        append " in format string at %L" to all strings but unexpected_element,
        use error as gfc_error formating string instead of
        "%s in format string at %L".  Formatting fixes.

--- gcc/fortran/io.c.jj 2019-05-23 12:57:17.762475649 +0200
+++ gcc/fortran/io.c 2019-06-11 12:24:23.155712025 +0200
@@ -596,12 +596,16 @@ token_to_string (format_token t)
 static bool
 check_format (bool is_input)
 {
-  const char *posint_required  = _("Positive width required");
-  const char *nonneg_required  = _("Nonnegative width required");
-  const char *unexpected_element  = _("Unexpected element %qc in format "
-      "string at %L");
-  const char *unexpected_end  = _("Unexpected end of format string");
-  const char *zero_width  = _("Zero width in format descriptor");
+  const char *posint_required
+    = G_("Positive width required in format string at %L");
+  const char *nonneg_required
+    = G_("Nonnegative width required in format string at %L");
+  const char *unexpected_element
+    = G_("Unexpected element %qc in format string at %L");
+  const char *unexpected_end
+    = G_("Unexpected end of format string in format string at %L");
+  const char *zero_width
+    = G_("Zero width in format descriptor in format string at %L");
 
   const char *error = NULL;
   format_token t, u;
@@ -621,7 +625,7 @@ check_format (bool is_input)
     goto fail;
   if (t != FMT_LPAREN)
     {
-      error = _("Missing leading left parenthesis");
+      error = G_("Missing leading left parenthesis in format string at %L");
       goto syntax;
     }
 
@@ -650,7 +654,8 @@ format_item_1:
   level++;
   goto format_item;
  }
-      error = _("Left parenthesis required after %<*%>");
+      error = G_("Left parenthesis required after %<*%> in format string "
+ "at %L");
       goto syntax;
 
     case FMT_POSINT:
@@ -681,7 +686,7 @@ format_item_1:
  goto fail;
       if (t != FMT_P)
  {
-  error = _("Expected P edit descriptor");
+  error = G_("Expected P edit descriptor in format string at %L");
   goto syntax;
  }
 
@@ -689,7 +694,8 @@ format_item_1:
 
     case FMT_P:
       /* P requires a prior number.  */
-      error = _("P descriptor requires leading scale factor");
+      error = G_("P descriptor requires leading scale factor in format "
+ "string at %L");
       goto syntax;
 
     case FMT_X:
@@ -783,7 +789,8 @@ data_desc:
   && t != FMT_F && t != FMT_E && t != FMT_EN && t != FMT_ES
   && t != FMT_D && t != FMT_G && t != FMT_RPAREN && t != FMT_SLASH)
  {
-  error = _("Comma required after P descriptor");
+  error = G_("Comma required after P descriptor in format string "
+     "at %L");
   goto syntax;
  }
       if (t != FMT_COMMA)
@@ -794,10 +801,11 @@ data_desc:
       if (t == FMT_ERROR)
  goto fail;
     }
-          if (t != FMT_F && t != FMT_E && t != FMT_EN && t != FMT_ES && t != FMT_D
-      && t != FMT_G && t != FMT_RPAREN && t != FMT_SLASH)
+  if (t != FMT_F && t != FMT_E && t != FMT_EN && t != FMT_ES
+      && t != FMT_D && t != FMT_G && t != FMT_RPAREN && t != FMT_SLASH)
     {
-      error = _("Comma required after P descriptor");
+      error = G_("Comma required after P descriptor in format string "
+ "at %L");
       goto syntax;
     }
  }
@@ -811,7 +819,8 @@ data_desc:
       t = format_lex ();
       if (t != FMT_POSINT)
  {
-  error = _("Positive width required with T descriptor");
+  error = G_("Positive width required with T descriptor in format "
+     "string at %L");
   goto syntax;
  }
       break;
@@ -894,7 +903,8 @@ data_desc:
   u = format_lex ();
   if (u == FMT_E)
     {
-      error = _("E specifier not allowed with g0 descriptor");
+      error = G_("E specifier not allowed with g0 descriptor in "
+ "format string at %L");
       goto syntax;
     }
   saved_token = u;
@@ -961,9 +971,7 @@ data_desc:
       if (u == FMT_ERROR)
  goto fail;
       if (u != FMT_E)
- {
-  saved_token = u;
- }
+ saved_token = u;
       else
  {
   u = format_lex ();
@@ -971,7 +979,8 @@ data_desc:
     goto fail;
   if (u != FMT_POSINT)
     {
-      error = _("Positive exponent width required");
+      error = G_("Positive exponent width required in format string "
+ "at %L");
       goto syntax;
     }
  }
@@ -1017,7 +1026,8 @@ data_desc:
     goto dtio_vlist;
   if (t != FMT_RPAREN)
     {
-      error = _("Right parenthesis expected at %C");
+      error = G_("Right parenthesis expected at %C in format string "
+ "at %L");
       goto syntax;
     }
   goto between_desc;
@@ -1058,7 +1068,8 @@ data_desc:
   /* Warn if -std=legacy, otherwise error.  */
   if (gfc_option.warn_std != 0)
     {
-      error = _("Period required in format specifier");
+      error = G_("Period required in format specifier in format "
+ "string at %L");
       goto syntax;
     }
   if (mode != MODE_FORMAT)
@@ -1132,9 +1143,7 @@ data_desc:
       if (t == FMT_ERROR)
  goto fail;
       if (t != FMT_PERIOD)
- {
-  saved_token = t;
- }
+ saved_token = t;
       else
  {
   t = format_lex ();
@@ -1262,7 +1271,7 @@ syntax:
   if (error == unexpected_element)
     gfc_error (error, error_element, &format_locus);
   else
-    gfc_error ("%s in format string at %L", error, &format_locus);
+    gfc_error (error, &format_locus);
 fail:
   rv = false;
 


        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: *ping* Re: [PATCH] PR fortran/89103 - Allow blank format items in format strings

Mark Eggleston
In reply to this post by Jakub Jelinek

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 <[hidden email]>
>>>      Mark Eggleston <[hidden email]>
>> 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  <[hidden email]>
     Mark Eggleston  <[hidden email]>

     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  <[hidden email]>
     Mark Eggleston  <[hidden email]>

     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.


--
https://www.codethink.co.uk/privacy.html


0006-Allow-blank-format-items-in-format-strings.patch (7K) Download Attachment