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.tho...@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
