[patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

The attached patch reverts a change I made for pr65456 which caused this
regression and adds a check for quotes farther down in the function.  This
avoids treating a '!' in a string as a comment and wiping the rest of the line.

I found the added code is needed because an interposing quote was falling
through and being changed into an empty space at around line 1393. (I do not
recall the history of why that space is being done)

The patch also updates test case continuation_13.f90. I found another compiler I
use for comparison interprets the continuation differently and is inserting a
space that is not there.  So the format at line 800 is handled differently now
with gfortran and the line 900 format example is added to test both cases with
and without a space just before the continuation.

I think now gfortran has this right, but I do not rule out that I am missing
some obscure rule.  This begs a question about the space character substituted
near line 1393 in scanner.c, but at the moment I do not think it is related.

I have also added a test for the the original problem reported in this PR to
avoid future repeating of the problem. I will provide a Changelog entry for the
testsuite changes.

Regression tested on x86-64-linux.  OK to trunk?  followed to 5.1?

Regards,

Jerry


2015-05-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/65903
	* io.c (format_lex): Change to NONSTRING when checking for
	possible doubled quote.
	* scanner.c (gfc_next_char_literal): Revert change from 64506,
	add a check for quotes, and return.


[-- Attachment #2: pr65903.diff --]
[-- Type: text/x-patch, Size: 2169 bytes --]

Index: gcc/fortran/io.c
===================================================================
--- gcc/fortran/io.c	(revision 223105)
+++ gcc/fortran/io.c	(working copy)
@@ -385,7 +385,7 @@ format_lex (void)
 
 	  if (c == delim)
 	    {
-	      c = next_char (INSTRING_NOWARN);
+	      c = next_char (NONSTRING);
 
 	      if (c == '\0')
 		{
Index: gcc/fortran/scanner.c
===================================================================
--- gcc/fortran/scanner.c	(revision 223105)
+++ gcc/fortran/scanner.c	(working copy)
@@ -1272,21 +1272,11 @@ restart:
 	 are still in a string and we are looking for a possible
 	 doubled quote and we end up here. See PR64506.  */
 
-      if (in_string)
+      if (in_string && c != '\n')
 	{
 	  gfc_current_locus = old_loc;
-
-	  if (c == '!')
-	    {
-	      skip_comment_line ();
-	      goto restart;
-	    }
-
-	  if (c != '\n')
-	    {
-	      c = '&';
-	      goto done;
-	    }
+	  c = '&';
+	  goto done;
 	}
 
       if (c != '!' && c != '\n')
@@ -1392,6 +1382,8 @@ restart:
 			     "Missing %<&%> in continued character "
 			     "constant at %C");
 	    }
+	  else if (!in_string && (c == '\'' || c == '"'))
+	      goto done;
 	  /* Both !$omp and !$ -fopenmp continuation lines have & on the
 	     continuation line only optionally.  */
 	  else if (openmp_flag || openacc_flag || openmp_cond_flag)
Index: gcc/testsuite/gfortran.dg/continuation_13.f90
===================================================================
--- gcc/testsuite/gfortran.dg/continuation_13.f90	(revision 223105)
+++ gcc/testsuite/gfortran.dg/continuation_13.f90	(working copy)
@@ -19,6 +19,8 @@ character(25) :: astring
     )
 800 format('This is actually ok.'& !comment
    ' end'  )
+900 format('This is actually ok.' & !comment
+   ' end'  )
 write(astring,100)
 if (astring.ne."This format is OK.") call abort
 write(astring,200)
@@ -34,6 +36,8 @@ if (astring.ne."This format now works.'") call abo
 write(astring,700)
 if (astring.ne."This format now works.'") call abort
 write(astring,800)
+if (astring.ne."This is actually ok.' end") call abort
+write(astring,900)
 if (astring.ne."This is actually ok. end") call abort
 
 end

[-- Attachment #3: pr65903.f90 --]
[-- Type: text/x-fortran, Size: 293 bytes --]

! { dg-do run }
! { dg-options "-std=gnu" }
! 
character(20) :: astring

100 format ("& notblank !")
200 format ("&          !")

write(astring,100)
if (astring.ne."& notblank !") call abort
!print *, astring
write(astring,200)
if (astring.ne."&          !") call abort
!print *, astring

end

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

* ping *

On 05/13/2015 06:58 PM, Jerry DeLisle wrote:
> The attached patch reverts a change I made for pr65456 which caused this
> regression and adds a check for quotes farther down in the function.  This
> avoids treating a '!' in a string as a comment and wiping the rest of the line.
> 
> I found the added code is needed because an interposing quote was falling
> through and being changed into an empty space at around line 1393. (I do not
> recall the history of why that space is being done)
> 
> The patch also updates test case continuation_13.f90. I found another compiler I
> use for comparison interprets the continuation differently and is inserting a
> space that is not there.  So the format at line 800 is handled differently now
> with gfortran and the line 900 format example is added to test both cases with
> and without a space just before the continuation.
> 
> I think now gfortran has this right, but I do not rule out that I am missing
> some obscure rule.  This begs a question about the space character substituted
> near line 1393 in scanner.c, but at the moment I do not think it is related.
> 
> I have also added a test for the the original problem reported in this PR to
> avoid future repeating of the problem. I will provide a Changelog entry for the
> testsuite changes.
> 
> Regression tested on x86-64-linux.  OK to trunk?  followed to 5.1?
> 
> Regards,
> 
> Jerry
> 
> 
> 2015-05-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> 
> 	PR fortran/65903
> 	* io.c (format_lex): Change to NONSTRING when checking for
> 	possible doubled quote.
> 	* scanner.c (gfc_next_char_literal): Revert change from 64506,
> 	add a check for quotes, and return.
> 

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Steve Kargl]

On Sat, May 16, 2015 at 07:52:38AM -0700, Jerry DeLisle wrote:
> * ping *
> 
> > 2015-05-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> > 
> > 	PR fortran/65903
> > 	* io.c (format_lex): Change to NONSTRING when checking for
> > 	possible doubled quote.
> > 	* scanner.c (gfc_next_char_literal): Revert change from 64506,
> > 	add a check for quotes, and return.
> > 

OK.

-- 
Steve

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

On 05/16/2015 08:11 AM, Steve Kargl wrote:
> On Sat, May 16, 2015 at 07:52:38AM -0700, Jerry DeLisle wrote:
>> * ping *
>>
>>> 2015-05-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
>>>
>>> 	PR fortran/65903
>>> 	* io.c (format_lex): Change to NONSTRING when checking for
>>> 	possible doubled quote.
>>> 	* scanner.c (gfc_next_char_literal): Revert change from 64506,
>>> 	add a check for quotes, and return.
>>>
> 
> OK.
> 

Thanks Steve,

Committed revision 223248.

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

On 05/16/2015 10:45 AM, Jerry DeLisle wrote:
--- snip ---

> Thanks Steve,
> 
> Committed revision 223248.
> 
> 

I had some time to play with this a little more this afternoon.

I am going to commit the following little patchlet that gives us the nice
warning we should have. (After full regression testing of course)

gfc -Wall continuation_13.f90
continuation_13.f90:22:4: Warning: Missing ‘&’ in continued character constant
at (1) [-Wampersand]
continuation_13.f90:24:4: Warning: Missing ‘&’ in continued character constant
at (1) [-Wampersand]


Index: scanner.c
===================================================================
--- scanner.c	(revision 223250)
+++ scanner.c	(working copy)
@@ -1383,7 +1383,12 @@
 			     "constant at %C");
 	    }
 	  else if (!in_string && (c == '\'' || c == '"'))
+	    {
+	      gfc_warning (OPT_Wampersand,
+			     "Missing %<&%> in continued character "
+			     "constant at %C");
 	      goto done;
+	    }
 	  /* Both !$omp and !$ -fopenmp continuation lines have & on the
 	     continuation line only optionally.  */
 	  else if (openmp_flag || openacc_flag || openmp_cond_flag)

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

On 05/16/2015 12:58 PM, Jerry DeLisle wrote:
> On 05/16/2015 10:45 AM, Jerry DeLisle wrote:
> --- snip ---
> 
>> Thanks Steve,
>>
>> Committed revision 223248.
>>
>>
> 
> I had some time to play with this a little more this afternoon.
> 
> I am going to commit the following little patchlet that gives us the nice
> warning we should have. (After full regression testing of course)

I changed my mind.  Adding this warning affects 22 test cases. Question for the
team.

Do we want this warning? If so, I will have to adjust each of the 22.

Jerry

> 
> gfc -Wall continuation_13.f90
> continuation_13.f90:22:4: Warning: Missing ‘&’ in continued character constant
> at (1) [-Wampersand]
> continuation_13.f90:24:4: Warning: Missing ‘&’ in continued character constant
> at (1) [-Wampersand]
> 
> 
> Index: scanner.c
> ===================================================================
> --- scanner.c	(revision 223250)
> +++ scanner.c	(working copy)
> @@ -1383,7 +1383,12 @@
>  			     "constant at %C");
>  	    }
>  	  else if (!in_string && (c == '\'' || c == '"'))
> +	    {
> +	      gfc_warning (OPT_Wampersand,
> +			     "Missing %<&%> in continued character "
> +			     "constant at %C");
>  	      goto done;
> +	    }
>  	  /* Both !$omp and !$ -fopenmp continuation lines have & on the
>  	     continuation line only optionally.  */
>  	  else if (openmp_flag || openacc_flag || openmp_cond_flag)
> 
> 
> 
> 
> 

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Mikael Morin]

Le 14/05/2015 03:58, Jerry DeLisle a écrit :
> The attached patch reverts a change I made for pr65456 which caused this
> regression and adds a check for quotes farther down in the function.  This
> avoids treating a '!' in a string as a comment and wiping the rest of the line.
> 
> I found the added code is needed because an interposing quote was falling
> through and being changed into an empty space at around line 1393. (I do not
> recall the history of why that space is being done)
> 
> The patch also updates test case continuation_13.f90. I found another compiler I
> use for comparison interprets the continuation differently and is inserting a
> space that is not there.  So the format at line 800 is handled differently now
> with gfortran and the line 900 format example is added to test both cases with
> and without a space just before the continuation.
> 
> I think now gfortran has this right, but I do not rule out that I am missing
> some obscure rule.  This begs a question about the space character substituted
> near line 1393 in scanner.c, but at the moment I do not think it is related.
> 
> I have also added a test for the the original problem reported in this PR to
> avoid future repeating of the problem. I will provide a Changelog entry for the
> testsuite changes.
> 
> Regression tested on x86-64-linux.  OK to trunk?  followed to 5.1?
> 
> Regards,
> 
> Jerry
> 
> 
> 2015-05-14  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> 
> 	PR fortran/65903
> 	* io.c (format_lex): Change to NONSTRING when checking for
> 	possible doubled quote.
> 	* scanner.c (gfc_next_char_literal): Revert change from 64506,
> 	add a check for quotes, and return.
> 
> Index: gcc/testsuite/gfortran.dg/continuation_13.f90
> ===================================================================
> --- gcc/testsuite/gfortran.dg/continuation_13.f90	(revision 223105)
> +++ gcc/testsuite/gfortran.dg/continuation_13.f90	(working copy)
> @@ -19,6 +19,8 @@ character(25) :: astring
>      )
>  800 format('This is actually ok.'& !comment
>     ' end'  )
> +900 format('This is actually ok.' & !comment
> +   ' end'  )
>  write(astring,100)
>  if (astring.ne."This format is OK.") call abort
>  write(astring,200)

Hello, is the new format labelled 900 correct?
It seems to me it is equivalent to
	900 format('This is actually ok.' ' end')
which is missing a comma, no?
This is a real question, I'm no format guru.

By the way, the existing 800 format is also not right, it shouldn't have
a comment, and it should have a '&' at the beginning of the next line.
Section 3.3.2.4 "Free form statement continuation", last paragraph has:

> If a character context is to be continued, an “&” shall be the last
> nonblank character on the line and shall not be followed by commentary.
> There shall be a later line that is not a comment; an “&” shall be the
> first nonblank character on the next such line and the statement
> continues with the next character following that “&”.

To be honest, I see little value in supporting continuation between the
two consecutive quotes coding one quote inside a string constant, it's
confusing.  But if it's supported, the above applies, doesn't it?


Regarding the patch itself, I haven't looked at it in great details yet,
but as it's a revert basically, it shouldn't do a lot of harm.
I see that the fixed form part isn't reverted, is it on purpose?

Mikael

Re: [patch, fortran] [5/6 Regression] Line continuation followed by comment character in string fails to compile

[Jerry DeLisle]

On 05/16/2015 08:17 AM, Mikael Morin wrote:
---- snip ----
>>
>> Index: gcc/testsuite/gfortran.dg/continuation_13.f90
>> ===================================================================
>> --- gcc/testsuite/gfortran.dg/continuation_13.f90	(revision 223105)
>> +++ gcc/testsuite/gfortran.dg/continuation_13.f90	(working copy)
>> @@ -19,6 +19,8 @@ character(25) :: astring
>>      )
>>  800 format('This is actually ok.'& !comment
>>     ' end'  )
>> +900 format('This is actually ok.' & !comment
>> +   ' end'  )
>>  write(astring,100)
>>  if (astring.ne."This format is OK.") call abort
>>  write(astring,200)
> 
> Hello, is the new format labelled 900 correct?
> It seems to me it is equivalent to
> 	900 format('This is actually ok.' ' end')
> which is missing a comma, no?
> This is a real question, I'm no format guru.

Yes, I agree one ought to use a comma.  See below.

> 
> By the way, the existing 800 format is also not right, it shouldn't have
> a comment, and it should have a '&' at the beginning of the next line.
> Section 3.3.2.4 "Free form statement continuation", last paragraph has:
> 
>> If a character context is to be continued, an “&” shall be the last
>> nonblank character on the line and shall not be followed by commentary.
>> There shall be a later line that is not a comment; an “&” shall be the
>> first nonblank character on the next such line and the statement
>> continues with the next character following that “&”.

The "right" way is to put the "&" at the beginning of the next line.  It is
common practice to allow this as an extension and it is assumed to begin at the
first non-space character encountered.  This does require special handling of
continued literal strings. This is why we try to give errors with -std=f95, etc.

$ gfc -std=f95 continuation_13.f90
continuation_13.f90:24:10:

    ' end'  )
          1
Error: GNU Extension: Missing comma at (1)
continuation_13.f90:41:17:

 write(astring,900)
                 1
Error: FORMAT label 900 at (1) not defined

I think these are cases where even the error detection is not perfect. User beware.

> 
> To be honest, I see little value in supporting continuation between the
> two consecutive quotes coding one quote inside a string constant, it's
> confusing.  But if it's supported, the above applies, doesn't it?
> 
> 
> Regarding the patch itself, I haven't looked at it in great details yet,
> but as it's a revert basically, it shouldn't do a lot of harm.
> I see that the fixed form part isn't reverted, is it on purpose?

Fixed form continuations are handled differently and I am not trying to address
issues there, if any.  As I have time, I will dabble further into this.  Getting
ready to move here from Southern California back to Washington State.

Regards,

Jerry