Message ID | 5185A43F.3000900@charter.net |
---|---|
State | New |
Headers | show |
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.
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.
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
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
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
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?
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
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
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: