From patchwork Sat Aug 6 15:39:06 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: [Fortran, OOP] PR 49638: [OOP] length parameter is ignored when overriding type bound character functions with constant length. Date: Sat, 06 Aug 2011 05:39:06 -0000 From: Thomas Koenig X-Patchwork-Id: 108789 Message-Id: <4E3D601A.7020401@netcologne.de> To: Janus Weil Cc: Mikael Morin , fortran@gcc.gnu.org, gcc-patches 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. */