[patch, fortran] PR45516 - [F08] allocatable components of recursive type

[Andre Vehreschild]

Hi Paul,

Just one small nit and one idea remaining:

The nit: You've missed one "same_type" at:

> Index: gcc/fortran/trans-array.c
> ===================================================================
> *** gcc/fortran/trans-array.c   (revision 241467)
> --- gcc/fortran/trans-array.c   (working copy)
> *************** structure_alloc_comps (gfc_symbol * der_
> *** 8421,8426 ****
> --- 8510,8516 ----
>               gfc_add_expr_to_block (&fnblock, tmp);
>             }
>           else if (c->attr.allocatable && !c->attr.proc_pointer
> +                  && !(c->ts.type == BT_DERIVED && der_type ==
> c->ts.u.derived) && (!(cmp_has_alloc_comps && c->as)

  ^^^^ here
>                        || c->attr.codimension))
>             {

The idea: One could think about doing the same "same_type" for the chunks in
===================================================================
*** gcc/fortran/trans-types.c   (revision 241467)
--- gcc/fortran/trans-types.c   (working copy)
*************** gfc_get_derived_type (gfc_symbol * deriv

which would eliminate evaluating the same checks thrice.

Besides that ok for trunk.

Thanks for the work.

Regards,
	Andre


On Tue, 25 Oct 2016 13:40:03 +0200
Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:

> Dear Andre,
> 
> Thanks for the review. Please find my responses below and the revised
> patch attached
> 
> With these changes, OK for trunk?
> 
> With respect to Dominique's testcase posted last night at 23:18, this
> is an issue for the feature which I will raise on clf. It seems to me
> that in the circumstances, a deep copy of the allocatable components
> should be made in the FROM expression in move_alloc but I am not sure.
> I will post a PR about this as well, after the patch is committed.
> 
> Best regards
> 
> Paul
> 
> ....snip....
> 
> >
> > So recursive types are defined to be simple recursive, i.e., not transitive?
> > type A == uses => type B == uses => type A  
> 
> This is caught by:
>       type(linked_list2), allocatable :: link
>                                        1
> Error: Derived type at (1) has not been previously defined and so
> cannot appear in a derived type definition
> andre.f90:10:28:
> 
>    type(linked_list1) :: test
>                             1
> Error: Symbol ‘test’ at (1) cannot have a type
> andre.f90:12:27:
> 
> So, in this implementation anyway, the answer to your question is 'yes'.
> 
> ....snip....
> 
> >> +           if (derived->attr.unlimited_polymorphic
> >> +               || derived->attr.abstract
> >> +               || !rdt)  
> >
> > When unlimited or abstract rdt can never be true. This is redundant here,
> > isn't it?  
> 
> No. Derived types that are not unlimited or abstract might not have
> recursive components.
> 
> ....snip....
> 
> >>         {
> >>           if (!c->attr.pointer && !c->attr.proc_pointer
> >> +          && !(c->attr.allocatable && (der == c->ts.u.derived))  
> >
> > The inner most pair of parentheses is unnecessary here.  
> 
> The parentheses have been removed.
> 
> ....snip....
> 
> > The indentation of the whole block above is wrong. The comment has 6 spaces
> > while it should have 2 only.  
> 
> Corrected. Well spotted.
> 
> ....snip....
> 
> > is_derived_type = c->ts.type == BT_DERIVED && der_type == c->ts.u.derived;
> >
> > and using that instead of repeatedly evaluating the expression?  
> 
> Done. I called it 'same_type'.
> >
> > <snip>
> >  
> >> *************** structure_alloc_comps (gfc_symbol * der_
> >> *** 8137,8142 ****
> >> --- 8141,8222 ----
> >>                                      build_int_cst (TREE_TYPE (comp), 0));
> >>               gfc_add_expr_to_block (&tmpblock, tmp);
> >>             }
> >> +         else if (c->attr.allocatable && !c->attr.codimension)
> >> +           {  
> >
> > How about putting also the creation of the descriptor into the body that is
> > guarded by the is_allocated, i.e. in pseudo-code make:  
> 
> ....snip....
> 
> done
> 
> >
> > Furthermore, why are you always using an array? We do now at code-generation
> > time whether the argument is a scalar or an array, don't we? This is more
> > for my understanding, then an actual comment.  
> 
> This is to limit the number of deallocators to 1 with a rank=1
> argument. This was the simplest solution.
> 
> >  
> >>   /* The size field is returned as an array index type.  Therefore treat
> >> Index: gcc/fortran/trans-types.c
> >> ===================================================================
> >> *** gcc/fortran/trans-types.c (revision 241467)
> >> --- gcc/fortran/trans-types.c (working copy)  
> 
> ....snip....
> 
> >> !               && !(c->attr.allocatable && (derived == c->ts.u.derived))  
> >
> > Spare parentheses above (derived == ..)  
> 
> All removed
> 
> 
> >> Index: gcc/testsuite/gfortran.dg/recursive_alloc_comp_3.f08
> >> ===================================================================
> >> *** gcc/testsuite/gfortran.dg/recursive_alloc_comp_3.f08      (revision 0)
> >> --- gcc/testsuite/gfortran.dg/recursive_alloc_comp_3.f08      (working
> >> copy) ***************  
> 
> ....Snip....
> 
> > This testcase fails only on segfaults, but never on wrong data. Can you
> > enhance it to e.g. make output() check that it is seeing 3.0 on the top
> > before the pop and 2.0 after?  
> 
> 
> Done


-- 
Andre Vehreschild * Email: vehre ad gmx dot de