[gfortran] Fix PR 15211

[gfortran] Fix PR 15211

[Tobias Schlüter]

Looking again at the code in gfc_conv_intrinsic_len it occurred to me 
that all strings in an array must be of the same length. Therefor this 
one line fix should do the trick. The test for arg->ref == NULL is 
clearly meant to separate the case of arg->ref == REF_SUBSTRING, but is 
meaningless in the case of REF_ARRAY or REF_COMPONENT (which I will look 
into after this is approved).

- Tobi

2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>

	* trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
	of strings.

Index: trans-intrinsic.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fortran/trans-intrinsic.c,v
retrieving revision 1.5
diff -u -p -r1.5 trans-intrinsic.c
--- trans-intrinsic.c   14 May 2004 15:32:01 -0000      1.5
+++ trans-intrinsic.c   31 May 2004 18:48:57 -0000
@@ -1874,7 +1874,8 @@ gfc_conv_intrinsic_len (gfc_se * se, gfc
        break;

      default:
-       if (arg->expr_type == EXPR_VARIABLE && arg->ref == NULL)
+       if (arg->expr_type == EXPR_VARIABLE
+           && (arg->ref == NULL || arg->ref->type == REF_ARRAY))
           {
             sym = arg->symtree->n.sym;
             decl = gfc_get_symbol_decl (sym);

Re: [gfortran] Fix PR 15211

[Tobias Schlüter]

Tobias Schlüter wrote:
> 2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>
> 
>     * trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
>     of strings.

I forgot to say: compiled and tested on i686-pc-linux with no new 
regressions. I also verified that the testcase from the PR passes.

- Tobi

Re: [gfortran] Fix PR 15211

[Paul Brook]

On Monday 31 May 2004 19:54, Tobias Schlüter wrote:
> Looking again at the code in gfc_conv_intrinsic_len it occurred to me
> that all strings in an array must be of the same length. Therefor this
> one line fix should do the trick. The test for arg->ref == NULL is
> clearly meant to separate the case of arg->ref == REF_SUBSTRING, but is
> meaningless in the case of REF_ARRAY or REF_COMPONENT (which I will look
> into after this is approved).
>
> 2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>
>
> 	* trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
> 	of strings.

You need to iterate over all the refs. You can have an substring (or component 
ref) of an arrayref.

Paul

Re: [gfortran] Fix PR 15211

[Tobias Schlüter]

Paul Brook wrote:
> On Monday 31 May 2004 19:54, Tobias Schlüter wrote:
> 
>>Looking again at the code in gfc_conv_intrinsic_len it occurred to me
>>that all strings in an array must be of the same length. Therefor this
>>one line fix should do the trick. The test for arg->ref == NULL is
>>clearly meant to separate the case of arg->ref == REF_SUBSTRING, but is
>>meaningless in the case of REF_ARRAY or REF_COMPONENT (which I will look
>>into after this is approved).
>>
>>2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>
>>
>>	* trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
>>	of strings.
> 
> 
> You need to iterate over all the refs. You can have an substring (or component 
> ref) of an arrayref.
> 

So, to make sure I understand correctly, checking (arg->ref == NULL || 
(arg->ref->next == NULL && arg->ref->type == REF_ARRAY)) should fix the 
PR, but leave us with a problem in the case of something like
  a(5)(1:3)
or
  a(5)%string
?

I can submit a patch with the revised check, but I don't think I can fix 
the other two examples, as my insight into the scalarizer is close to 
non-existant.

- Tobi

Re: [gfortran] Fix PR 15211

[Paul Brook]

On Monday 14 June 2004 17:39, Tobias Schlüter wrote:
> Paul Brook wrote:
> > On Monday 31 May 2004 19:54, Tobias Schlüter wrote:
> >>Looking again at the code in gfc_conv_intrinsic_len it occurred to me
> >>that all strings in an array must be of the same length. Therefor this
> >>one line fix should do the trick. The test for arg->ref == NULL is
> >>clearly meant to separate the case of arg->ref == REF_SUBSTRING, but is
> >>meaningless in the case of REF_ARRAY or REF_COMPONENT (which I will look
> >>into after this is approved).
> >>
> >>2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>
> >>
> >>	* trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
> >>	of strings.
> >
> > You need to iterate over all the refs. You can have an substring (or
> > component ref) of an arrayref.
>
> So, to make sure I understand correctly, checking (arg->ref == NULL ||
> (arg->ref->next == NULL && arg->ref->type == REF_ARRAY)) should fix the

Yes, that would work. Patch approved with that change.

I imagine we'll want to end up with something like:

/* (1) */
if ( arg->expr_type == EXPR_VARIABLE)
  {
  gfc_ref *ref = arg->ref;
  while (ref && we_have_magic_for_this_ref (ref))
  {
    switch (ref->type)
    {
    case SUBSTRING_REF:
      <Evaluate the bounds of the substring and return that.>
    case COMPONENT_REF:
      <Find the component decl>
    case ARRAY_REF:
      <nop>
    }
    ref = ref->next;
  }
  if (!ref)
    {
    <pull the length out of the appropriate decl >
    return <...>;
    } 
}

/* (2) */
val = evaluate_arbirary_expression(arg)
return string_length(val)
}

This would allow us to traverse multiple component/array/substring refs.

> PR, but leave us with a problem in the case of something like
>   a(5)(1:3)
> or
>   a(5)%string
> ?
>
> I can submit a patch with the revised check, but I don't think I can fix
> the other two examples, as my insight into the scalarizer is close to
> non-existant.

I see these as two separate issues:
1) Special casing known expressions. This includes your patch.
2) Teaching the general case how to evaluate array expressions. This involves 
either using the existing function argument evaluation routines, or talking 
to the scalarizer directly.

It's possible that (2) may be unnecessary if (1) is sufficiently complete.

Paul

Re: [gfortran] Fix PR 15211

[Tobias Schlüter]

Paul Brook wrote:
>>
>>So, to make sure I understand correctly, checking (arg->ref == NULL ||
>>(arg->ref->next == NULL && arg->ref->type == REF_ARRAY)) should fix the
> 
> Yes, that would work. Patch approved with that change.

Below is what I committed after testing. I also added a check for this 
to execute/intrinsic_len.f90.

- Tobi

Index: ChangeLog
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fortran/ChangeLog,v
retrieving revision 1.73
diff -u -p -r1.73 ChangeLog
--- ChangeLog   14 Jun 2004 16:04:39 -0000      1.73
+++ ChangeLog   14 Jun 2004 18:46:33 -0000
@@ -1,3 +1,9 @@
+2004-05-31  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>
+
+       PR fortran/15211
+       * trans-intrinsic.c (gfc_conv_intrinsic_len): Deal with arrays
+       of strings.
+
  2004-06-14  Tobias Schlueter  <tobias.schlueter@physik.uni-muenchen.de>

         PR fortran/15510
Index: trans-intrinsic.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fortran/trans-intrinsic.c,v
retrieving revision 1.6
diff -u -p -r1.6 trans-intrinsic.c
--- trans-intrinsic.c   12 Jun 2004 17:34:45 -0000      1.6
+++ trans-intrinsic.c   14 Jun 2004 18:46:33 -0000
@@ -1874,8 +1874,12 @@ gfc_conv_intrinsic_len (gfc_se * se, gfc
        break;

      default:
-       if (arg->expr_type == EXPR_VARIABLE && arg->ref == NULL)
+       if (arg->expr_type == EXPR_VARIABLE && arg->ref == NULL
+           || (arg->ref->next == NULL && arg->ref->type == REF_ARRAY))
           {
+           /* This doesn't catch all cases.
+              See http://gcc.gnu.org/ml/fortran/2004-06/msg00165.html
+              and the surrounding thread.  */
             sym = arg->symtree->n.sym;
             decl = gfc_get_symbol_decl (sym);
             if (decl == current_function_decl && sym->attr.function

Re: [gfortran] Fix PR 15211

[Tobias Schlüter]

Tobias Schlüter wrote:
> Below is what I committed after testing. I also added a check for this 
> to execute/intrinsic_len.f90.
> 

If (as I think) && has a higher precedence than ||, my previous patch 
was wrong, so I committed this, which adds parentheses and readability 
to the expression.

- Tobi

Index: trans-intrinsic.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/fortran/trans-intrinsic.c,v
retrieving revision 1.7
diff -u -p -r1.7 trans-intrinsic.c
--- trans-intrinsic.c   14 Jun 2004 18:50:42 -0000      1.7
+++ trans-intrinsic.c   14 Jun 2004 19:16:39 -0000
@@ -1874,8 +1874,9 @@ gfc_conv_intrinsic_len (gfc_se * se, gfc
        break;

      default:
-       if (arg->expr_type == EXPR_VARIABLE && arg->ref == NULL
-           || (arg->ref->next == NULL && arg->ref->type == REF_ARRAY))
+       if (arg->expr_type == EXPR_VARIABLE
+           && (arg->ref == NULL || (arg->ref->next == NULL
+                                    && arg->ref->type == REF_ARRAY)))
           {
             /* This doesn't catch all cases.
                See http://gcc.gnu.org/ml/fortran/2004-06/msg00165.html