From patchwork Sat Aug 6 15:39:06 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thomas Koenig X-Patchwork-Id: 108789 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id D84A2B6F72 for ; Sun, 7 Aug 2011 01:39:28 +1000 (EST) Received: (qmail 19440 invoked by alias); 6 Aug 2011 15:39:26 -0000 Received: (qmail 19420 invoked by uid 22791); 6 Aug 2011 15:39:25 -0000 X-SWARE-Spam-Status: No, hits=-0.1 required=5.0 tests=AWL, BAYES_50, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cc-smtpout3.netcologne.de (HELO cc-smtpout3.netcologne.de) (89.1.8.213) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 06 Aug 2011 15:39:11 +0000 Received: from cc-smtpin3.netcologne.de (cc-smtpin3.netcologne.de [89.1.8.203]) by cc-smtpout3.netcologne.de (Postfix) with ESMTP id 35CC1123D0; Sat, 6 Aug 2011 17:39:10 +0200 (CEST) Received: from [192.168.0.197] (xdsl-78-35-137-25.netcologne.de [78.35.137.25]) by cc-smtpin3.netcologne.de (Postfix) with ESMTPSA id D55F911E89; Sat, 6 Aug 2011 17:39:06 +0200 (CEST) Message-ID: <4E3D601A.7020401@netcologne.de> Date: Sat, 06 Aug 2011 17:39:06 +0200 From: Thomas Koenig User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.2.17) Gecko/20110414 SUSE/3.1.10 Thunderbird/3.1.10 MIME-Version: 1.0 To: Janus Weil CC: Mikael Morin , fortran@gcc.gnu.org, gcc-patches Subject: Re: [Patch, Fortran, OOP] PR 49638: [OOP] length parameter is ignored when overriding type bound character functions with constant length. References: <4E3C5A69.3000305@netcologne.de> <201108052344.53971.mikael.morin@sfr.fr> In-Reply-To: Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi Janus, > 2011/8/5 Mikael Morin: >> On Friday 05 August 2011 23:02:33 Thomas Koenig wrote: >>>> The extra >>>> argument controls whether we check variable symbols for equality or >>>> just their names. For the overriding checks it is sufficient to check >>>> for names, because the arguments of the overriding procedure are >>>> required to have the same names as in the base procedure. >>> >>> Could you explain for which cases this test is too strict? >> For dummy arguments. If they are "corresponding" (same position, same name), >> they should compare equal. Cf the PR. > > The string length expressions of overridden procedures have to be > identical, but with exchanged dummy arguments. Since the dummy > arguments of overridden procedures must have the same name as in the > base procedure, it is sufficient the check for equal names. Checking > for equal symbols would be too strict. I just tested the following patch: without any regressions. Can anybody think of a case where the names can be identical, but the variables different? (I can't). Maybe we can relax the test that way and get rid of the large number of changes for gfc_dep_compare_expr everywhere (which I confess I dislike, but I can hardly find fault with something that I have done only yesterday, although the number of changes was much smaller there :-) > 1) I have moved 'check_typebound_override' to interface.c and prefixed > it with 'gfc_'. OK. > 2) I have added the 'var_name_only flag' also to > gfc_are_identical_variables, gfc_dep_compare_functions, > identical_array_ref, check_section_vs_section and gfc_is_same_range. I > hope there is nothing else I missed. See above; could we avoid that? > 3) I have made 'gfc_are_identical_variables' static and removed the > gfc prefix (it does not seem to be used outside of dependency.c). OK. > 4) I have made 'gfc_is_same_range' static and removed the gfc prefix > (there is only a commented out reference to it in trans-array.c, so I > commented out the declaration in dependency.h, too). Also I removed > the 'def' argument, which gets always passed a '0'. OK. > As Thomas mentions, certain cases are still not handled correctly > (e.g. A+B+C vs C+B+A, and other mathematical transformations), but I > hope they are sufficiently exotic (so that we can wait for bug reports > to roll in). In addition I expect people to declare overridden > procedures analogously to the base procedure, and not use e.g. > len=3*(x+1) in one case and len=3*x+3 in the other. Not OK. It is wrong to assume that expressions are unequal because we cannot prove they are equal, with all the limitations that we currently have. This will introduce rejects-valid bugs. Please change + /* Check string length. */ + if (proc_target->result->ts.type == BT_CHARACTER + && proc_target->result->ts.u.cl && old_target->result->ts.u.cl + && gfc_dep_compare_expr (proc_target->result->ts.u.cl->length, + old_target->result->ts.u.cl->length, + true) != 0) to something like (untested) + /* Check string length. */ + if (proc_target->result->ts.type == BT_CHARACTER + && proc_target->result->ts.u.cl && old_target->result->ts.u.cl + { + int len_comparision; + len_comparison = gfc_dep_compare_expr (proc_target->result->ts.u.cl->length, + old_target->result->ts.u.cl->length); + if (len_comparison != 0 && len_comparison != -2) ... Alternatively, you could raise an error for 1 and -1 and warn only for -2 (... may be different). Regards Thomas Index: dependency.c =================================================================== --- dependency.c (Revision 177487) +++ dependency.c (Arbeitskopie) @@ -123,7 +123,7 @@ gfc_are_identical_variables (gfc_expr *e1, gfc_exp { gfc_ref *r1, *r2; - if (e1->symtree->n.sym != e2->symtree->n.sym) + if (strcmp(e1->symtree->n.sym->name, e2->symtree->n.sym->name)) return false; /* Volatile variables should never compare equal to themselves. */