[Fortran,OOP] PR 49638: [OOP] length parameter is ignored when overriding type bound character functions with constant length.

Hi Janus,

> 2011/8/5 Mikael Morin<mikael.morin@sfr.fr>:
>> 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

On Saturday 06 August 2011 17:39:06 Thomas Koenig wrote:
> > 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.
In the PR at
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49638#c8
I quote the standard:

4.5.7.3 (type-bound procedure overriding) has:
• Either both shall be subroutines or both shall be functions having the same
result characteristics (12.3.3).

12.3.3 (Characteristics of function results):
If a type parameter of a function result or a bound of a function result array
is not a constant expression, the
exact dependence on the entities in the expression is a characteristic


So the standards is more restrictive than expression values being the same. It 
requires _the exact same dependence on the entities_. My reading of this is 
that 3*(x+1) vs 3*x+3 is right to be rejected, same for (a+b)+c vs a+(b+c). 
The only worrying case that I see is the one you pointed out: a+b+c vs c+b+a 
(without brackets).


Mikael

>> 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.
> In the PR at
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49638#c8
> I quote the standard:
>
> 4.5.7.3 (type-bound procedure overriding) has:
> • Either both shall be subroutines or both shall be functions having the same
> result characteristics (12.3.3).
>
> 12.3.3 (Characteristics of function results):
> If a type parameter of a function result or a bound of a function result array
> is not a constant expression, the
> exact dependence on the entities in the expression is a characteristic
>
>
> So the standards is more restrictive than expression values being the same. It
> requires _the exact same dependence on the entities_. My reading of this is
> that 3*(x+1) vs 3*x+3 is right to be rejected, same for (a+b)+c vs a+(b+c).
> The only worrying case that I see is the one you pointed out: a+b+c vs c+b+a
> (without brackets).

Huh, I don't see what is so different between

1) 3*(x+1)   vs  3*x+3   and
2) a+b+c  vs  c+b+a

In both cases the expressions look different at first sight, but can
be transformed into each other mathematically. So I'd say they are
mathematically equivalent, although the spelled-out representations of
these expressions differ.

The question is how you interpret the standard's formulation of "exact
dependence on the entities in the expression". Naively I would have
taken this to mean the *mathematical* dependence (which can be
represented by different actual expressions). But I'm fine with your
interpretation, too, which will make life even easier for us.

Cheers,
Janus

Hi Thomas,


>> 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:
>
> 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.  */
>
> without any regressions.  Can anybody think of a case where the names can be
> identical, but the variables different?  (I can't).

Well, I'd say this can only happen if both variables reside in
different namespaces (i.e. different modules or procedures).


> 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 :-)

Ok, I don't like the large number of changes either, but I assumed
they were necessary.

I have to admit I'm not aware of all the cases that
'gfc_dep_compare_expr' was intended for originally. I was only trying
to re-use it for checking overriding procedures, which seems to work
very well, except for the "variable names vs. symbols" issue. If you
tell me it's fine to only check for variable names everywhere, this is
of course fine.

Btw, the fact that your patch has no regressions does not necessarily
mean that there are no cases where it could fail. It could just mean
that the testsuite does not cover these cases.

Cheers,
Janus

Am 06.08.2011 18:16, schrieb Janus Weil:
>> without any regressions.  Can anybody think of a case where the names can be
>> >  identical, but the variables different?  (I can't).
> Well, I'd say this can only happen if both variables reside in
> different namespaces (i.e. different modules or procedures).
>

gfc_are_identical variables is only called from within 
gfc_dep_compare_expr.  It makes no sense to call this function
to compare expressions from different statements, unless one has 
carefully analyzed that no intervening assignment to the variables has 
taken place.  Comparing across namespaces makes even less sense.

So yes, I think it is enough if we compare the variable names, and
document this in a commtent.

 > I have to admit I'm not aware of all the cases that
 > 'gfc_dep_compare_expr' was intended for originally. I was only trying
 > to re-use it for checking overriding procedures, which seems to work
 > very well, except for the "variable names vs. symbols" issue. If you
 > tell me it's fine to only check for variable names everywhere, this is
 > of course fine.

Well,  I also wrote the function, so maybe I can claim a little bit of
authority here on the way it was originally meant to be ;-)

 > Btw, the fact that your patch has no regressions does not necessarily
 > mean that there are no cases where it could fail. It could just mean
 > that the testsuite does not cover these cases.

Of course.  However, when analysis and regression test agree, it is 
usually a good sign :-)

On Saturday 06 August 2011 18:06:58 Janus Weil wrote:
> >> 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.
> > 
> > In the PR at
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49638#c8
> > I quote the standard:
> > 
> > 4.5.7.3 (type-bound procedure overriding) has:
> > • Either both shall be subroutines or both shall be functions having the
> > same result characteristics (12.3.3).
> > 
> > 12.3.3 (Characteristics of function results):
> > If a type parameter of a function result or a bound of a function result
> > array is not a constant expression, the
> > exact dependence on the entities in the expression is a characteristic
> > 
> > 
> > So the standards is more restrictive than expression values being the
> > same. It requires _the exact same dependence on the entities_. My
> > reading of this is that 3*(x+1) vs 3*x+3 is right to be rejected, same
> > for (a+b)+c vs a+(b+c). The only worrying case that I see is the one you
> > pointed out: a+b+c vs c+b+a (without brackets).
> 
> Huh, I don't see what is so different between
> 
> 1) 3*(x+1)   vs  3*x+3   and
> 2) a+b+c  vs  c+b+a
> 
> In both cases the expressions look different at first sight, but can
> be transformed into each other mathematically. So I'd say they are
> mathematically equivalent, although the spelled-out representations of
> these expressions differ.
I was looking at the standard, because I was not so sure myself.
Here is what is written (7.1.5.2.4):

Once the interpretation of a numeric intrinsic operation is established, the 
processor may evaluate any mathematically equivalent expression, provided that 
the integrity of parentheses is not violated.

Two expressions of a numeric type are mathematically equivalent if, for all 
possible values of their primaries, theirmathematical values are equal.


So parentheses have to be respected; other than that anything is possible.
This is about the evaluation of expressions though, not about the "dependences 
on entities".


> 
> The question is how you interpret the standard's formulation of "exact
> dependence on the entities in the expression". 
That is the question.

> Naively I would have
> taken this to mean the *mathematical* dependence (which can be
> represented by different actual expressions). But I'm fine with your
> interpretation, too, which will make life even easier for us.
Yes, my interpretation is somewhat biased towards ease of implementation. ;-)


Mikael

>>> without any regressions.  Can anybody think of a case where the names can
>>> be
>>> >  identical, but the variables different?  (I can't).
>>
>> Well, I'd say this can only happen if both variables reside in
>> different namespaces (i.e. different modules or procedures).
>>
>
> gfc_are_identical variables is only called from within gfc_dep_compare_expr.
>  It makes no sense to call this function
> to compare expressions from different statements, unless one has carefully
> analyzed that no intervening assignment to the variables has taken place.
>  Comparing across namespaces makes even less sense.
>
> So yes, I think it is enough if we compare the variable names, and
> document this in a commtent.

Actually, on second thought, I disagree.

For the original usage cases of gfc_dep_compare_expr, I'm not sure if
one can guarantee the expressions to be in the same namespace.

However, for the overriding checks, both expressions are guaranteed to
be in *different* namespaces (namely: two different procedures). And
as Mikael noted, it is crucial wether the symbols in the expressions
are dummy arguments or not:

1) Dummies are guaranteed to have equal names in overridden
procedures, so we can just compare names.

2) Non-dummies could have the same name, but still sit in different
namespaces, so for them we really have to check for equal symbols!


Here is a variant of the original test case from the PR, which will be
accepted if we only check for names (but it should actually be
rejected):


module world

  implicit none

  type :: world_1
   contains
     procedure, nopass :: string => w1_string
  end type

  type, extends(world_1) :: world_2
   contains
     procedure, nopass :: string => w2_string
  end type

contains

  function w1_string (m)
    integer, parameter :: n = 5
    integer, intent(in) :: m
    character(n+m) :: w1_string
    w1_string = "world"
  end function

  function w2_string (m)
    integer, parameter :: n = 6
    integer, intent(in) :: m
    character(n+m) :: w2_string
    w2_string = "world2"
  end function

end module


Cheers,
Janus

> Here is a variant of the original test case from the PR, which will be
> accepted if we only check for names (but it should actually be
> rejected):
>
>
> module world
>
>  implicit none
>
>  type :: world_1
>   contains
>     procedure, nopass :: string => w1_string
>  end type
>
>  type, extends(world_1) :: world_2
>   contains
>     procedure, nopass :: string => w2_string
>  end type
>
> contains
>
>  function w1_string (m)
>    integer, parameter :: n = 5
>    integer, intent(in) :: m
>    character(n+m) :: w1_string
>    w1_string = "world"
>  end function
>
>  function w2_string (m)
>    integer, parameter :: n = 6
>    integer, intent(in) :: m
>    character(n+m) :: w2_string
>    w2_string = "world2"
>  end function
>
> end module

Sorry, now I have to disagree with my own earlier claims: In this
example, the 'n' variables will of course be simplified to
EXPR_CONSTANTs, so the name checking does not apply to them.

And since the string length can not depend on local variables which
are *not* constant, name checking should still be fine!

Now, if Thomas says it's fine for the other cases, too, then it seems
we can really get away with a much simpler patch. Hope we're not
missing anything, though ...

Cheers,
Janus

On Saturday 06 August 2011 19:10:09 Janus Weil wrote:
> Now, if Thomas says it's fine for the other cases, too, then it seems
> we can really get away with a much simpler patch. Hope we're not
> missing anything, though ...
> 

What about this case: two module variables from two different modules?




module world1

 implicit none

 integer :: n

 type :: t1
  contains
    procedure, nopass :: string => w1_string
 end type

contains

 function w1_string (m)
   integer, intent(in) :: m
   character(n) :: w1_string
   w1_string = "world"
 end function


end module world1


module world2 

 use world1, only : t1

 implicit none

 integer :: n

 type, extends(t1) :: t2
  contains
    procedure, nopass :: string => w2_string
 end type

contains

 function w2_string (m)
   integer, intent(in) :: m
   character(n) :: w2_string
   w2_string = "world2"
 end function

end module world2

On Sat, Aug 06, 2011 at 06:45:36PM +0200, Mikael Morin wrote:
> On Saturday 06 August 2011 18:06:58 Janus Weil wrote:
> > >> 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.
> > > 
> > > In the PR at
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49638#c8
> > > I quote the standard:
> > > 
> > > 4.5.7.3 (type-bound procedure overriding) has:
> > > ? Either both shall be subroutines or both shall be functions having the
> > > same result characteristics (12.3.3).
> > > 
> > > 12.3.3 (Characteristics of function results):
> > > If a type parameter of a function result or a bound of a function result
> > > array is not a constant expression, the
> > > exact dependence on the entities in the expression is a characteristic
> > > 
> > > 
> > > So the standards is more restrictive than expression values being the
> > > same. It requires _the exact same dependence on the entities_. My
> > > reading of this is that 3*(x+1) vs 3*x+3 is right to be rejected, same
> > > for (a+b)+c vs a+(b+c). The only worrying case that I see is the one you
> > > pointed out: a+b+c vs c+b+a (without brackets).
> > 
> > Huh, I don't see what is so different between
> > 
> > 1) 3*(x+1)   vs  3*x+3   and
> > 2) a+b+c  vs  c+b+a
> > 
> > In both cases the expressions look different at first sight, but can
> > be transformed into each other mathematically. So I'd say they are
> > mathematically equivalent, although the spelled-out representations of
> > these expressions differ.
> I was looking at the standard, because I was not so sure myself.
> Here is what is written (7.1.5.2.4):
> 
> Once the interpretation of a numeric intrinsic operation is established, the 
> processor may evaluate any mathematically equivalent expression, provided that 
> the integrity of parentheses is not violated.
> 
> Two expressions of a numeric type are mathematically equivalent if, for all 
> possible values of their primaries, theirmathematical values are equal.
> 
> 
> So parentheses have to be respected; other than that anything is possible.

See Note 7.18.  X*(Y-Z) -> X*Y - X*Z is a forbidden transformation
(there is no noted restriction on Z > 0).

a + b + c -> b + a + c -> b + (a + c) is a sequence of allowable
transformations.