Re: [Patch, fortran] PR41600 - [OOP] SELECT TYPE with associate-name => exp: Arrays not supported

[Tobias Burnus <burnus at net-b dot de>]

Dear Paul,

thanks for the patch.

Paul Richard Thomas wrote:
> + /* Transfer the selector typespec to the associate name.  */
> +
> + copy_ts_from_selector_to_associate (gfc_expr *expr1, gfc_expr *expr2)
> + {
 I think it is not obvious which type spec is which. Maybe you could 
add a "(expr1)" and "(expr2)"  in the comment. (Alternatively, one could 
rename expr1 and expr2.)

> +   if (expr2->ts.type == BT_CLASS
> + 	&&  CLASS_DATA (expr2)->as
> + 	&&  expr2->ref&&  expr2->ref->type == REF_ARRAY)
> +     {
> +       if (expr2->ref->u.ar.type == AR_FULL)
> + 	expr2->rank = CLASS_DATA (expr2)->as->rank;
> +       else if (expr2->ref->u.ar.type == AR_SECTION)
> + 	expr2->rank = expr2->ref->u.ar.dimen;
> +     }

I have a bad feeling about that one for code like:
  dt%class(1:2)
  class%class(1:2)
  dt(1:2)%class
  class(1:2)%class
I fear that at least one of those will fail. In any case, assuming that 
- if the last ref is BT_CLASS - the expr->ref is the right one, looks 
wrong. But I might have missed some fine print and it is guaranteed to 
be always the correct.

> +   /* Logic is a LOT clearer with separate functions for class and derived
> +      type temporaries! There are not many more lines of code either.  */
>      if (ts->type == BT_CLASS)
> !     tmp = select_class_set_tmp (ts);
> !   else
> !     tmp = select_derived_set_tmp (ts);

While I concur with the comment, I think one should remove it. As patch 
explanation it makes sense, but as committed it is not helpful.

>      gfc_add_type (tmp->n.sym, ts, NULL);
>
> ! /* Copy across the array spec to the selector, taking care as to
> !    whether or not it is a class object or not.  */

The indention looks wrong.

> (iii) The error that is thrown in resolve_assoc_var is necessary
> because wrong code is produced at the moment since the size of the
> declared type, rather than the dynamic type, is used for allocation of
> the temporary.  The necessary machinery is in place to fix this and I
> will do so soon

I assume that's:
> !       gfc_error ("CLASS selector at %L needs a temporary which is not "
> ! 		 "yet implemented",&target->where);

But I think one should also look into:
> !      TODO Understand why class scalar expressions must be excluded.  */
> !   if (sym->assoc&&  !(sym->ts.type == BT_CLASS&&  e->rank == 0))

Overall, the patch looks okay - I am just unsure about the expr2->ref 
usage in copy_ts_from_selector_to_associate.

Tobias