public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed
       [not found] <op.wti2pupfa8ed4e@dellschleppa.fritz.box>
@ 2013-03-07 10:48 ` Tilo Schwarz
  2013-03-07 11:46   ` Tobias Burnus
  0 siblings, 1 reply; 5+ messages in thread
From: Tilo Schwarz @ 2013-03-07 10:48 UTC (permalink / raw)
  To: fortran; +Cc: gcc-patches

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

Hi,

this patch fixes PR 48618.

Built and regtested on Linux 3.2.0-4-686-pae.

Thanks for input and corrections to Tobias Burnus.


Regards,

	Tilo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: negative_unit.diff --]
[-- Type: text/diff; name=negative_unit.diff, Size: 2801 bytes --]

diff --git a/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90 b/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90
new file mode 100644
index 0000000..6446436
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90
@@ -0,0 +1,21 @@
+! { dg-do run }
+! PR48618 - Negative unit number in OPEN(...) is sometimes allowed
+!
+! Test originally from Janne Blomqvist in PR:
+! http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48618
+
+program nutest
+    implicit none
+    integer id, ios
+
+    open(newunit=id, file="foo.txt", iostat=ios)
+    if (ios /= 0) call abort
+
+    open(id, file="bar.txt", iostat=ios)
+    if (ios /= 0) call abort
+
+    close(id, status="delete")
+
+    open(-10, file="foo.txt", iostat=ios)
+    if (ios == 0) call abort
+end program nutest
diff --git a/libgfortran/ChangeLog b/libgfortran/ChangeLog
index 54ac573..f8e3f52 100644
--- a/libgfortran/ChangeLog
+++ b/libgfortran/ChangeLog
@@ -1,3 +1,9 @@
+2013-03-06  Tilo Schwarz  <tilo@tilo-schwarz.de>
+
+	PR libfortran/48618
+	* io/open.c (st_open): Raise error for unit number < 0 only if
+	unit number does not exist already.
+
 2013-02-21  Janne Blomqvist  <jb@gcc.gnu.org>
 
 	PR libfortran/30162
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index d9cfde8..df4808e 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -818,10 +818,6 @@ st_open (st_parameter_open *opp)
 
   flags.convert = conv;
 
-  if (!(opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT) && opp->common.unit < 0)
-    generate_error (&opp->common, LIBERROR_BAD_OPTION,
-		    "Bad unit number in OPEN statement");
-
   if (flags.position != POSITION_UNSPECIFIED
       && flags.access == ACCESS_DIRECT)
     generate_error (&opp->common, LIBERROR_BAD_OPTION,
@@ -841,6 +837,18 @@ st_open (st_parameter_open *opp)
       flags.position = POSITION_APPEND;
     }
 
+  if (!(opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT) && opp->common.unit < 0)
+    {
+      u = find_unit (opp->common.unit);
+      if (u == NULL) /* negative unit and no unit found */
+        generate_error (&opp->common, LIBERROR_BAD_OPTION,
+                        "Bad unit number in OPEN statement");
+      /* check for previous error, otherwise unit won't be unlocked later */
+      else if ((opp->common.flags & IOPARM_LIBRETURN_MASK)
+	       != IOPARM_LIBRETURN_OK)
+	     unlock_unit (u);
+    }
+
   if (flags.position == POSITION_UNSPECIFIED)
     flags.position = POSITION_ASIS;
 
@@ -849,7 +857,8 @@ st_open (st_parameter_open *opp)
       if ((opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT))
 	opp->common.unit = get_unique_unit_number(opp);
 
-      u = find_or_create_unit (opp->common.unit);
+      if (u == NULL)
+        u = find_or_create_unit (opp->common.unit);
       if (u->s == NULL)
 	{
 	  u = new_unit (opp, u, &flags);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed
  2013-03-07 10:48 ` [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed Tilo Schwarz
@ 2013-03-07 11:46   ` Tobias Burnus
  2013-03-07 16:35     ` Tilo Schwarz
  0 siblings, 1 reply; 5+ messages in thread
From: Tobias Burnus @ 2013-03-07 11:46 UTC (permalink / raw)
  To: Tilo Schwarz; +Cc: fortran, gcc-patches

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

Hi,

Tilo Schwarz wrote:
> this patch fixes PR 48618.
> Built and regtested on Linux 3.2.0-4-686-pae.

Thanks for the patch, which mostly looks okay.

A few remarks:

* Do not create a diff of the ChangeLog but include it separately at the 
top of the patch. Reason: It's easier to read and if another patch has 
been applied before, your patch won't apply.

* A ChangeLog for the testcase is missing.


* Coding style: If 8 spaces have accumulated, replace them by one tab, i.e.
-      u = find_or_create_unit (opp->common.unit);
+      if (u == NULL)
+        u = find_or_create_unit (opp->common.unit);

Here, the last line should use a "tab". (The other important rule is to 
have maximally 80 character per source line.)

See also http://gcc.gnu.org/wiki/FormattingCodeForGCC

(Consistent coding styles makes it easier to read the code. The style 
might not always the best or to the personal taste, but still 
consistency helps. For instance, function names always start in column 1 
and have a " " before the "(". Hence, one can do "grep '^funcname ' *.c 
to find the function.)



* Comment style:
+      if (u == NULL) /* negative unit and no unit found */
+        generate_error (&opp->common, LIBERROR_BAD_OPTION,
+                        "Bad unit number in OPEN statement");
+      /* check for previous error, otherwise unit won't be unlocked 
later */

To quote the comment style from 
http://www.gnu.org/prep/standards/html_node/Comments.html:

"Please put two spaces after the end of a sentence in your comments, so 
that the Emacs sentence commands will work. Also, please write complete 
sentences and capitalize the first word. If a lower-case identifier 
comes at the beginning of a sentence, donÂ’t capitalize it! Changing the 
spelling makes it a different identifier. If you donÂ’t like starting a 
sentence with a lower case letter, write the sentence differently (e.g., 
“The identifier lower-case is …”)."

(The existing code does not always follow the style, but one should at 
least try, even if there is some leeway ;-)


* Regarding your patch itself. I think it is easier to read if one moves 
the code a bit further down as I did in the attachment. What do you think?


* Copyright assignment: You mentioned that you have emailed(§) the form 
back to the FSF. Was this the actual copyright-assignment form which the 
FSF sent to you by mail? Or was it 'only' request form? Can you tell us, 
when the FSF has acknowledged the arrival of the copyright assignment?


Tobias


(§) Side note: The FSF now also allows to send the copyright form back 
by FAX or (scanned) by email, but only if you are residing in the USA or 
in Germany. Residents of other countries still have to return it by 
(air,surface) mail. Cf. 
http://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html

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

diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index d9cfde8..19fab1d 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -817,12 +817,8 @@ st_open (st_parameter_open *opp)
     }
 
   flags.convert = conv;
 
-  if (!(opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT) && opp->common.unit < 0)
-    generate_error (&opp->common, LIBERROR_BAD_OPTION,
-		    "Bad unit number in OPEN statement");
-
   if (flags.position != POSITION_UNSPECIFIED
       && flags.access == ACCESS_DIRECT)
     generate_error (&opp->common, LIBERROR_BAD_OPTION,
 		    "Cannot use POSITION with direct access files");
@@ -847,10 +843,18 @@ st_open (st_parameter_open *opp)
   if ((opp->common.flags & IOPARM_LIBRETURN_MASK) == IOPARM_LIBRETURN_OK)
     {
       if ((opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT))
 	opp->common.unit = get_unique_unit_number(opp);
+      else if (opp->common.unit < 0)
+	{
+	  u = find_unit (opp->common.unit);
+	  if (u == NULL) /* Negative unit and no NEWUNIT-created unit found.  */
+	    generate_error (&opp->common, LIBERROR_BAD_OPTION,
+			    "Bad unit number in OPEN statement");
+	}
 
-      u = find_or_create_unit (opp->common.unit);
+      if (u == NULL)
+	u = find_or_create_unit (opp->common.unit);
       if (u->s == NULL)
 	{
 	  u = new_unit (opp, u, &flags);
 	  if (u != NULL)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed
  2013-03-07 11:46   ` Tobias Burnus
@ 2013-03-07 16:35     ` Tilo Schwarz
  2013-03-19 11:19       ` Tobias Burnus
  0 siblings, 1 reply; 5+ messages in thread
From: Tilo Schwarz @ 2013-03-07 16:35 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: fortran, gcc-patches

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

On Thu, 07 Mar 2013 12:46:10 +0100, Tobias Burnus <burnus@net-b.de> wrote:

> Hi,
>
> Tilo Schwarz wrote:
>> this patch fixes PR 48618.
>> Built and regtested on Linux 3.2.0-4-686-pae.
>
> Thanks for the patch, which mostly looks okay.
>
> A few remarks:

Thank you for the feedback.

I incorporated all remarks into the new attached patch.

> * Copyright assignment: You mentioned that you have emailed(§) the form
> back to the FSF. Was this the actual copyright-assignment form which the
> FSF sent to you by mail? Or was it 'only' request form?

It was the actual copyright-assignment form.

> Can you tell us,
> when the FSF has acknowledged the arrival of the copyright assignment?

Yes, I'll do.


Regards,
	Tilo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: negative_unit.diff --]
[-- Type: text/diff; name=negative_unit.diff, Size: 2182 bytes --]

2013-03-06  Tilo Schwarz  <tilo@tilo-schwarz.de>

	PR libfortran/48618
	* io/open.c (st_open): Raise error for unit number < 0 only if
	unit number does not exist already.
	* gcc/testsuite/gfortran.dg/open_negative_unit_1.f90: New.

diff --git a/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90 b/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90
new file mode 100644
index 0000000..6446436
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/open_negative_unit_1.f90
@@ -0,0 +1,21 @@
+! { dg-do run }
+! PR48618 - Negative unit number in OPEN(...) is sometimes allowed
+!
+! Test originally from Janne Blomqvist in PR:
+! http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48618
+
+program nutest
+    implicit none
+    integer id, ios
+
+    open(newunit=id, file="foo.txt", iostat=ios)
+    if (ios /= 0) call abort
+
+    open(id, file="bar.txt", iostat=ios)
+    if (ios /= 0) call abort
+
+    close(id, status="delete")
+
+    open(-10, file="foo.txt", iostat=ios)
+    if (ios == 0) call abort
+end program nutest
diff --git a/libgfortran/io/open.c b/libgfortran/io/open.c
index d9cfde8..19fab1d 100644
--- a/libgfortran/io/open.c
+++ b/libgfortran/io/open.c
@@ -818,10 +818,6 @@ st_open (st_parameter_open *opp)
 
   flags.convert = conv;
 
-  if (!(opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT) && opp->common.unit < 0)
-    generate_error (&opp->common, LIBERROR_BAD_OPTION,
-		    "Bad unit number in OPEN statement");
-
   if (flags.position != POSITION_UNSPECIFIED
       && flags.access == ACCESS_DIRECT)
     generate_error (&opp->common, LIBERROR_BAD_OPTION,
@@ -848,8 +844,16 @@ st_open (st_parameter_open *opp)
     {
       if ((opp->common.flags & IOPARM_OPEN_HAS_NEWUNIT))
 	opp->common.unit = get_unique_unit_number(opp);
+      else if (opp->common.unit < 0)
+	{
+	  u = find_unit (opp->common.unit);
+	  if (u == NULL) /* Negative unit and no NEWUNIT-created unit found.  */
+	    generate_error (&opp->common, LIBERROR_BAD_OPTION,
+			    "Bad unit number in OPEN statement");
+	}
 
-      u = find_or_create_unit (opp->common.unit);
+      if (u == NULL)
+	u = find_or_create_unit (opp->common.unit);
       if (u->s == NULL)
 	{
 	  u = new_unit (opp, u, &flags);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed
  2013-03-07 16:35     ` Tilo Schwarz
@ 2013-03-19 11:19       ` Tobias Burnus
  2013-03-19 21:22         ` Tilo Schwarz
  0 siblings, 1 reply; 5+ messages in thread
From: Tobias Burnus @ 2013-03-19 11:19 UTC (permalink / raw)
  To: Tilo Schwarz; +Cc: fortran, gcc-patches

Am 07.03.2013 17:35, schrieb Tilo Schwarz:
> On Thu, 07 Mar 2013 12:46:10 +0100, Tobias Burnus <burnus@net-b.de> 
> wrote:
>> Tilo Schwarz wrote:
>>> this patch fixes PR 48618.
>>> Built and regtested on Linux 3.2.0-4-686-pae.
>>
>> Thanks for the patch, which mostly looks okay.
>> A few remarks:
>
> Thank you for the feedback.
> I incorporated all remarks into the new attached patch.

The patch looks good and is okay for the 4.9 trunk. Thanks for your efforts!

If you commit yourself, you need to recall to split the ChangeLog into 
the parts which go into libgfortran/ChangeLog and 
gcc/testsuite/ChangeLog. For the commit log, use "svn log|less" to see 
what others do for the commit log.

If you want me to commit this (and the other) patch instead, please tell me.

Tobias

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed
  2013-03-19 11:19       ` Tobias Burnus
@ 2013-03-19 21:22         ` Tilo Schwarz
  0 siblings, 0 replies; 5+ messages in thread
From: Tilo Schwarz @ 2013-03-19 21:22 UTC (permalink / raw)
  To: Tobias Burnus; +Cc: fortran, gcc-patches

On Tue, 19 Mar 2013 12:01:12 +0100, Tobias Burnus <burnus@net-b.de> wrote:

> Am 07.03.2013 17:35, schrieb Tilo Schwarz:
>> On Thu, 07 Mar 2013 12:46:10 +0100, Tobias Burnus <burnus@net-b.de>  
>> wrote:
>>> Tilo Schwarz wrote:
>>>> this patch fixes PR 48618.
>>>> Built and regtested on Linux 3.2.0-4-686-pae.
>>>
>>> Thanks for the patch, which mostly looks okay.
>>> A few remarks:
>>
>> Thank you for the feedback.
>> I incorporated all remarks into the new attached patch.
>
> The patch looks good and is okay for the 4.9 trunk. Thanks for your  
> efforts!
>
> If you commit yourself, you need to recall to split the ChangeLog into  
> the parts which go into libgfortran/ChangeLog and  
> gcc/testsuite/ChangeLog. For the commit log, use "svn log|less" to see  
> what others do for the commit log.
>
> If you want me to commit this (and the other) patch instead, please tell  
> me.

Yes, I'm glad if you commit the two patches. I'll set up the svn for  
future patches.

Regards,

	Tilo

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2013-03-19 21:22 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <op.wti2pupfa8ed4e@dellschleppa.fritz.box>
2013-03-07 10:48 ` [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed Tilo Schwarz
2013-03-07 11:46   ` Tobias Burnus
2013-03-07 16:35     ` Tilo Schwarz
2013-03-19 11:19       ` Tobias Burnus
2013-03-19 21:22         ` Tilo Schwarz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).