diff mbox

[libgfortran] PR56743 Namelist bug with comment and no blank

Message ID 5185A43F.3000900@charter.net
State New
Headers show

Commit Message

Jerry DeLisle May 5, 2013, 12:13 a.m. UTC 
The attached patch resolves this PR by treating '!', the Fortran comment mark,
as a valid separator between values.  Thus, when encountered while reading a
value, the read is ended normally with whatever value was encountered.  This is
an extension beyond the Standards which require a separator before a comment mark.

I must emphasize that extensions like this are no guarantee of portability
across compilers.  Users should use well formed namelists always.

Regression tested on x86-64. Test case attached.  OK for trunk?

Regards,

Jerry

Comments

Jerry DeLisle May 5, 2013, 12:33 a.m. UTC | #1 
On 05/04/2013 05:13 PM, Jerry DeLisle wrote:
> The attached patch resolves this PR by treating '!', the Fortran comment mark,
> as a valid separator between values.  Thus, when encountered while reading a
> value, the read is ended normally with whatever value was encountered.  This is
> an extension beyond the Standards which require a separator before a comment mark.
> 
> I must emphasize that extensions like this are no guarantee of portability
> across compilers.  Users should use well formed namelists always.
> 
> Regression tested on x86-64. Test case attached.  OK for trunk?
> 
> Regards,
> 
> Jerry
> 

BTW Here is the ChangeLog entry

2013-05-04  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR libfortran/56743
	* io/list_read.c (read_integer): Treat '!' as a separator.
	(read_real): Likewise.
Steve Kargl May 5, 2013, 1:30 a.m. UTC | #2 
On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
>  
>  	CASE_SEPARATORS:	/* Not a repeat count.  */
>  	case EOF:
> +	case '!':

      if (c == '!')
         gfc_warning("GNU Fortran extension: accepting a possibly "
                 "corrupted namelist");
>  	  goto done;
>  
>  	default:
> @@ -890,6 +891,7 @@ read_integer (st_parameter_dt *dtp, int length)
>  
>  	CASE_SEPARATORS:
>  	case EOF:
> +	case '!':

     see above

>  	  goto done;
>  
>  	default:
> @@ -1489,6 +1491,7 @@ read_real (st_parameter_dt *dtp, void * dest, int
>  
>  	CASE_SEPARATORS:
>  	case EOF:
> +	case '!':
>            if (c != '\n' && c != ',' && c != '\r' && c != ';')
>  	    unget_char (dtp, c);
>  	  goto done;
> @@ -1558,6 +1561,7 @@ read_real (st_parameter_dt *dtp, void * dest, int
>  
>  	CASE_SEPARATORS:
>  	case EOF:
> +	case '!':

     see above

>  	  goto done;
>  
>  	case '.':
> @@ -1618,6 +1622,7 @@ read_real (st_parameter_dt *dtp, void * dest, int
>  
>  	CASE_SEPARATORS:
>  	case EOF:
> +	case '!':

    see above.

>  	  goto done;
>  
>  	default:

I would prefer that gfortran issues an error.
Issuing a warning is acceptable.
Patch as is not OK IMHO.

PS: A vendor extension should be documented in the manual.
Jerry DeLisle May 5, 2013, 5:25 a.m. UTC | #3 
On 05/04/2013 06:30 PM, Steve Kargl wrote:
> On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
>>  
>>  	CASE_SEPARATORS:	/* Not a repeat count.  */
>>  	case EOF:
>> +	case '!':
> 
>       if (c == '!')
>          gfc_warning("GNU Fortran extension: accepting a possibly "
>                  "corrupted namelist");

--- SNIP ---

> I would prefer that gfortran issues an error.
> Issuing a warning is acceptable.
> Patch as is not OK IMHO.
> 
> PS: A vendor extension should be documented in the manual.
>

I don't see much point in issuing a warning if we accept it.  I can just as
easily make it an error with something like "A value separator is required
before a namelist comment" and be done with trying to second guess whether
someone is using namelists right or not.

An error would be most consistent across all variable kinds.

Any other opinions out there?

Jerry
Janus Weil May 5, 2013, 7:39 a.m. UTC | #4 
2013/5/5 Jerry DeLisle <jvdelisle@charter.net>:
> On 05/04/2013 06:30 PM, Steve Kargl wrote:
>> On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
>>>
>>>      CASE_SEPARATORS:        /* Not a repeat count.  */
>>>      case EOF:
>>> +    case '!':
>>
>>       if (c == '!')
>>          gfc_warning("GNU Fortran extension: accepting a possibly "
>>                  "corrupted namelist");
>
> --- SNIP ---
>
>> I would prefer that gfortran issues an error.
>> Issuing a warning is acceptable.
>> Patch as is not OK IMHO.
>>
>> PS: A vendor extension should be documented in the manual.
>>
>
> I don't see much point in issuing a warning if we accept it.  I can just as
> easily make it an error with something like "A value separator is required
> before a namelist comment" and be done with trying to second guess whether
> someone is using namelists right or not.
>
> An error would be most consistent across all variable kinds.
>
> Any other opinions out there?

How about throwing an error only with -std=f2008 and friends, and
accepting it as an extension with -std=gnu?

I find it a bit surprising that the standard requires a blank there,
since it doesn't require a blank in such a situation in Fortran source
code (right?). If there is no technical reason to reject it, we might
as well always allow it if you ask me. But also a run-time error would
certainly be an improvement over the current behavior ...

Thanks for the patch,
Janus
Tobias Burnus May 5, 2013, 12:41 p.m. UTC | #5 
Janus Weil wrote:
> 2013/5/5 Jerry DeLisle <jvdelisle@charter.net>:
>> On 05/04/2013 06:30 PM, Steve Kargl wrote:
>>> On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
>>>>       CASE_SEPARATORS:        /* Not a repeat count.  */
>>>>       case EOF:
>>>> +    case '!':
>>>        if (c == '!')
>>>           gfc_warning("GNU Fortran extension: accepting a possibly "
>>>                   "corrupted namelist");
>>>
>>> I would prefer that gfortran issues an error.
>>> Issuing a warning is acceptable.
>>> Patch as is not OK IMHO.
>>>
>>> PS: A vendor extension should be documented in the manual.
>>>
>> I don't see much point in issuing a warning if we accept it.  I can just as
>> easily make it an error with something like "A value separator is required
>> before a namelist comment" and be done with trying to second guess whether
>> someone is using namelists right or not.
>>
>> An error would be most consistent across all variable kinds.
>>
>> Any other opinions out there?
> How about throwing an error only with -std=f2008 and friends, and
> accepting it as an extension with -std=gnu?

I personally do not like modified run-time behaviour, depending on 
-std=, as it can be very confusing. Especially as it depends only on the 
compile-time options used for the Fortran main program and not on the 
flags used for compiling the file. I also do not like the warning.

On the other hand, I am fine with either accepting "!" comments without 
blank as vendor extension and with unconditionally printing an error.

In any case, the patch is an improvement to the current situation of 
silently ignoring that entry. (Printing an error would be likewise an 
improvement.)

Tobias
Steve Kargl May 5, 2013, 12:47 p.m. UTC | #6 
On Sat, May 04, 2013 at 10:25:19PM -0700, Jerry DeLisle wrote:
> On 05/04/2013 06:30 PM, Steve Kargl wrote:
> > On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
> >>  
> >>  	CASE_SEPARATORS:	/* Not a repeat count.  */
> >>  	case EOF:
> >> +	case '!':
> > 
> >       if (c == '!')
> >          gfc_warning("GNU Fortran extension: accepting a possibly "
> >                  "corrupted namelist");
> 
> --- SNIP ---
> 
> > I would prefer that gfortran issues an error.
> > Issuing a warning is acceptable.
> > Patch as is not OK IMHO.
> > 
> > PS: A vendor extension should be documented in the manual.
> >
> 
> I don't see much point in issuing a warning if we accept it.

Point 1.  If the standard requires a valid separator before !
          then gfortran should complain about the nonconforming
          namelist.  Silently violating the standard just seems
          wrong to me.

Point 2. By issuing the warning, the user will be alerted to the
         nonconforming namelist and may then be motivated to
         fix the problem for portability.

>  I can just as
> easily make it an error with something like "A value separator is required
> before a namelist comment" and be done with trying to second guess whether
> someone is using namelists right or not.

Issuing an error is my preference.  Issuing a warning is also
acceptable.  Silently aceepting the code seems wrong.  Do we
know what other compilers do?
Jerry DeLisle May 5, 2013, 5:45 p.m. UTC | #7 
On 05/05/2013 05:47 AM, Steve Kargl wrote:
> On Sat, May 04, 2013 at 10:25:19PM -0700, Jerry DeLisle wrote:
>> On 05/04/2013 06:30 PM, Steve Kargl wrote:
>>> On Sat, May 04, 2013 at 05:13:51PM -0700, Jerry DeLisle wrote:
>>>>  
>>>>  	CASE_SEPARATORS:	/* Not a repeat count.  */
>>>>  	case EOF:
>>>> +	case '!':
>>>
>>>       if (c == '!')
>>>          gfc_warning("GNU Fortran extension: accepting a possibly "
>>>                  "corrupted namelist");
>>
>> --- SNIP ---
>>
>>> I would prefer that gfortran issues an error.
>>> Issuing a warning is acceptable.
>>> Patch as is not OK IMHO.
>>>
>>> PS: A vendor extension should be documented in the manual.
>>>
>>
>> I don't see much point in issuing a warning if we accept it.
> 
> Point 1.  If the standard requires a valid separator before !
>           then gfortran should complain about the nonconforming
>           namelist.  Silently violating the standard just seems
>           wrong to me.
> 
> Point 2. By issuing the warning, the user will be alerted to the
>          nonconforming namelist and may then be motivated to
>          fix the problem for portability.
> 
>>  I can just as
>> easily make it an error with something like "A value separator is required
>> before a namelist comment" and be done with trying to second guess whether
>> someone is using namelists right or not.
> 
> Issuing an error is my preference.  Issuing a warning is also
> acceptable.  Silently aceepting the code seems wrong.  Do we
> know what other compilers do?
> 

All,

Just to clarify, the issue is not a missing blank, its really a missing valid
separator which can be any one of ' ', ',', ';', '/', '\n', '\r' .

Jerry
Janne Blomqvist May 5, 2013, 5:59 p.m. UTC | #8 
On Sun, May 5, 2013 at 8:25 AM, Jerry DeLisle <jvdelisle@charter.net> wrote:
> I don't see much point in issuing a warning if we accept it.  I can just as
> easily make it an error with something like "A value separator is required
> before a namelist comment" and be done with trying to second guess whether
> someone is using namelists right or not.
>
> An error would be most consistent across all variable kinds.
>
> Any other opinions out there?

- I don't like the idea of printing a runtime warning either - users
are likely to ignore such things anyway.

- Different behavior with different -std= switches should be avoided
if possible - it just makes coding and testing more difficult than it
already is.

So my preference would be to generate an error, or if this is a common
extension in other compilers, accept it silently (and add a note about
it in the manual).



--
Janne Blomqvist
diff mbox

Patch

Index: list_read.c
===================================================================
--- list_read.c	(revision 198600)
+++ list_read.c	(working copy)
@@ -840,6 +840,7 @@  read_integer (st_parameter_dt *dtp, int length)
 
 	CASE_SEPARATORS:	/* Not a repeat count.  */
 	case EOF:
+	case '!':
 	  goto done;
 
 	default:
@@ -890,6 +891,7 @@  read_integer (st_parameter_dt *dtp, int length)
 
 	CASE_SEPARATORS:
 	case EOF:
+	case '!':
 	  goto done;
 
 	default:
@@ -1489,6 +1491,7 @@  read_real (st_parameter_dt *dtp, void * dest, int
 
 	CASE_SEPARATORS:
 	case EOF:
+	case '!':
           if (c != '\n' && c != ',' && c != '\r' && c != ';')
 	    unget_char (dtp, c);
 	  goto done;
@@ -1558,6 +1561,7 @@  read_real (st_parameter_dt *dtp, void * dest, int
 
 	CASE_SEPARATORS:
 	case EOF:
+	case '!':
 	  goto done;
 
 	case '.':
@@ -1618,6 +1622,7 @@  read_real (st_parameter_dt *dtp, void * dest, int
 
 	CASE_SEPARATORS:
 	case EOF:
+	case '!':
 	  goto done;
 
 	default: