Committed as revision 241539.
Thanks for all the help, Andre and Dominique.
Paul
On 25 October 2016 at 16:09, Andre Vehreschild <ve...@gmx.de> wrote:
> 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
--
The difference between genius and stupidity is; genius has its limits.
Albert Einstein
