Re: [Patch/gfortran] PR16939, 17192,17193,17202,18689,18890 &21297

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

Paul Thomas wrote:
> 2005-05-25  Paul Thomas  <pault@gcc.gnu.org>
> 
>  PR fortran/16939
>  PR fortran/17192
>  PR fortran/17193
>  PR fortran/17202
>  PR fortran/18689
>  PR fortran/18890
>  PR fortran/21297
>  * fortran/trans-array.c (gfc_conv_resolve_dependencies): Add string
>  length to temp_ss for character pointer array assignments.
>  * fortran/trans-expr.c (gfc_conv_variable): Correct errors in
>  dereferencing of characters and character pointers.
>  * fortran/trans-expr.c (gfc_conv_function_call): Provide string
>  length as return argument for various kinds of handling of return.
>  Return a char[]* temporary for character pointer functions and
>  dereference the temporary upon return.
> 
> Bootstrapped and regtested on i686/RH9.  OK for 4.1?

I think this is ok, but there are a few cases where whitespace-only changes
have slipped in, and the patch doesn't apply with the weird formatting your
mailer gives it, so I'm not sure I'm not missing something.  Can you somehow
get a cleaner patch out of your setup?

> I have realised that one more test case is needed to check that dependencies 
> are indeed resolved.  This will follow with the testsuite changelog entry. 
> In fact, might it be an idea to extend these tests to cover the intrinsic 
> and derived types?  It might be nice, for a change, to have testsuite 
> entries for the things that work, rather than those that are broken!
> 
> Paul Thomas
> 
> Enclosures: patch, character_pointer_assign.f90, character_pointer_dummy.f90 
> and character_pointer_func.f90.
> 

> --- 354,396 ----
>          se->expr = gfc_build_addr_expr (NULL, se->expr);
>        }
>      return;
> !  }
> 
> +
> +       /* Dereference the expression, where needed. Since characters
> +   are entirely different from other types, they are treated
> +   separately.  */
> +       if (sym->ts.type == BT_CHARACTER)
> +  {
> +           /* Dereference character pointer dummy arguments
> +       or results.  */
> +    if ((sym->attr.pointer || sym->attr.allocatable)
> +        && ((sym->attr.dummy)
> +     || (sym->attr.function
> +     || sym->attr.result)))
> +      se->expr = gfc_build_indirect_ref (se->expr);
> +  }
> +       else
> +  {
> +           /* Dereference non-charcter scalar dummy arguments.  */
> +    if ((sym->attr.dummy) && (!sym->attr.dimension))
> +      se->expr = gfc_build_indirect_ref (se->expr);
> +
> +           /* Dereference scalar hidden result.  */
> +    if ((gfc_option.flag_f2c && sym->ts.type == BT_COMPLEX)
> +        && (sym->attr.function || sym->attr.result)
> +        && (!sym->attr.dimension))
> +      se->expr = gfc_build_indirect_ref (se->expr);
> +
> +           /* Dereference non-character pointer variables.
> +       These must be dummys or results or scalars.  */
> +    if ((sym->attr.pointer || sym->attr.allocatable)
> +        && ((sym->attr.dummy)
> +     || (sym->attr.function || sym->attr.result)
> +     || (!sym->attr.dimension)))
> +      se->expr = gfc_build_indirect_ref (se->expr);
> +  }
> +
>         ref = expr->ref;
>       }
> 

I think you're using parentheses much too liberally, making the code harder to
read.

> *************** gfc_conv_function_call (gfc_se * se, gfc
> *** 1097,1102 ****
> --- 1119,1127 ----
>                 /* Access the previously obtained result.  */
>                 gfc_conv_tmp_array_ref (se);
>                 gfc_advance_se_ss_chain (se);
> +
> +        /* Bundle in the string length.  */
> +        se->string_length=len;
                            ^^ lacks spaces
>                 return;
>               }
>    }

> --- 1161,1206 ----
>        gfc_conv_descriptor_stride (info->descriptor, gfc_rank_cst[0]);
>      gfc_add_modify_expr (&se->pre, tmp,
>             convert (TREE_TYPE (tmp), integer_zero_node));
> +
>      /* Pass the temporary as the first argument.  */
>      tmp = info->descriptor;
>      tmp = gfc_build_addr_expr (NULL, tmp);
>      arglist = gfc_chainon_list (arglist, tmp);
> +
> +    /* Add string length to argument list.  */
> +    if (sym->ts.type == BT_CHARACTER)
> +      {
> +        sym->ts.cl->backend_decl = len;
> +        arglist = gfc_chainon_list (arglist,
> +           convert (gfc_charlen_type_node, len));
> +      }
> +
>    }
>         else if (sym->ts.type == BT_CHARACTER)
>    {
> !
> !    /* Pass the string length.  */
>      sym->ts.cl->backend_decl = len;
>      type = gfc_get_character_type (sym->ts.kind, sym->ts.cl);
>      type = build_pointer_type (type);
> 
> !    /* Return an address to a char[4]* temporary for character pointers. 
> */
> !    if (sym->attr.pointer || sym->attr.allocatable)
> !      {
> !        /* Build char[4] * pstr.  */

Shouldn't that be char[0:len-1] in both comments instead of 4?

> !        tmp = fold_build2 (MINUS_EXPR, gfc_charlen_type_node, len,
> !      convert (gfc_charlen_type_node, integer_one_node));

build_int_cst (gfc_charlen_type_node, 1)

> !        tmp = build_range_type (gfc_array_index_type, gfc_index_zero_node, 
> tmp);
> !        tmp = build_array_type (gfc_character1_type_node, tmp);
> !        var = gfc_create_var (build_pointer_type (tmp), "pstr");
> !
> !        /* Provide an address expression for the function arguments.  */
> !        var = gfc_build_addr_expr (NULL, var);
> !      }
> !    else
> !      {
> !        var = gfc_conv_string_tmp (se, type, len);
> !      }
>      arglist = gfc_chainon_list (arglist, var);
>      arglist = gfc_chainon_list (arglist,
>             convert (gfc_charlen_type_node, len));


Thanks,
- Tobi