From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4515 invoked by alias); 7 Mar 2013 11:46:41 -0000 Received: (qmail 4494 invoked by uid 22791); 7 Mar 2013 11:46:40 -0000 X-SWARE-Spam-Status: No, hits=-3.0 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_DNSWL_NONE X-Spam-Check-By: sourceware.org Received: from mx02.qsc.de (HELO mx02.qsc.de) (213.148.130.14) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Mar 2013 11:46:14 +0000 Received: from archimedes.net-b.de (port-92-195-1-51.dynamic.qsc.de [92.195.1.51]) by mx02.qsc.de (Postfix) with ESMTP id 18B7E278B5; Thu, 7 Mar 2013 12:46:11 +0100 (CET) Message-ID: <51387E02.60403@net-b.de> Date: Thu, 07 Mar 2013 11:46:00 -0000 From: Tobias Burnus User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130215 Thunderbird/17.0.3 MIME-Version: 1.0 Newsgroups: gmane.comp.gcc.fortran,gmane.comp.gcc.patches To: Tilo Schwarz CC: fortran@gcc.gnu.org, gcc-patches@gcc.gnu.org Subject: Re: [Patch, libfortran, 2nd version] PR 48618 - Negative unit number in OPEN(...) is sometimes allowed References: In-Reply-To: Content-Type: multipart/mixed; boundary="------------010008050402050605080600" Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-03/txt/msg00289.txt.bz2 This is a multi-part message in MIME format. --------------010008050402050605080600 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Content-length: 2810 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 --------------010008050402050605080600 Content-Type: text/x-patch; name="open.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="open.diff" Content-length: 1327 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) --------------010008050402050605080600--