Re: [Patch, fortran] 99307 - FAIL: gfortran.dg/class_assign_4.f90 execution test

[Tobias Burnus <tobias@codesourcery.com>]

On 11.04.21 09:05, Paul Richard Thomas wrote:
> Tobias noticed a major technical fault with the resubmission below: I
> forgot to attach the patch :-(

LGTM. Plus as remarked in the first review: 'trans-expr_c' typo needs to
be fixed (ChangeLog).

Tobias

>
> Please find it attached this time.
>
> Paul
>
> On Tue, 6 Apr 2021 at 18:08, Paul Richard Thomas
> <paul.richard.thomas@gmail.com <mailto:paul.richard.thomas@gmail.com>>
> wrote:
>
>     Hi Tobias,
>
>     I believe that the attached fixes the problems that you found with
>     gfc_find_and_cut_at_last_class_ref.
>
>     I will test:
>        type1%type%array_class2 → NULL is returned  (why?)
>        class1%type%array_class2 → ts = class1 but array2_class is used
>     later on (ups!)
>        class1%...%scalar_class2 → ts = class1 but scalar_class2 is used
>
>     The ChangeLogs remain the same, apart from the date.
>
>     Regtests OK on FC33/x86_64.
>
>     Paul
>
>
>     On Mon, 29 Mar 2021 at 14:58, Tobias Burnus
>     <tobias@codesourcery.com <mailto:tobias@codesourcery.com>> wrote:
>
>         Hi all,
>
>         as preremark I want to note that the testcase class_assign_4.f90
>         was added for PR83118/PR96012 (fixes problems in handling
>         class objects, Dec 18, 2020)
>         and got revised for PR99124 (class defined operators, Feb 23,
>         2021).
>         Both patches were then also applied to GCC 9 and 10.
>
>         On 26.03.21 17:30, Paul Richard Thomas via Gcc-patches wrote:
>         > This patch comes in two versions: submit.diff with
>         Change.Logs or
>         > submit2.diff with Change2.Logs.
>         > The first fixes the problem by changing array temporaries
>         from class
>         > expressions into class temporaries. This permits the use of
>         > gfc_get_class_from_expr to obtain the vptr for these
>         temporaries and all
>         > the good things that come with that when handling dynamic
>         types. The second
>         > part of the fix is to use the array element length from the
>         class
>         > descriptor, when reallocating on assignment. This is needed
>         because the
>         > vptr is being set too early. I will set about trying to
>         track down why this
>         > is happening and fix it after release.
>         >
>         > The second version does the same as the first but puts in
>         place a load of
>         > tidying up that is permitted by the fix to class array
>         temporaries.
>
>         > I couldn't readily see how to prepare a testcase - ideas?
>         > Both regtest on FC33/x86_64. The first was tested by
>         Dominique (see the
>         > PR). OK for master?
>
>         Typo – underscore-'c' should be a dot-'c' – both changelog files
>
>         >       * trans-expr_c (gfc_trans_scalar_assign): Make use of
>         pre and
>
>         I think the second longer version is nicer in general, but at
>         least for
>         GCC 9/GCC10 the first version is simpler and, hence, less
>         error prone.
>
>         As you only ask about mainline, I would prefer the second one.
>
>         However, I am not happy about gfc_find_and_cut_at_last_class_ref:
>
>         > + of refs following. If ts is non-null the cut is at the
>         class entity
>         > + or component that is followed by an array reference, which
>         is not +
>         > an element. */ ... + + if (ts) + { + if (e->symtree + &&
>         > e->symtree->n.sym->ts.type == BT_CLASS) + *ts =
>         > &e->symtree->n.sym->ts; + else + *ts = NULL; + } + for (ref
>         = e->ref;
>         > ref; ref = ref->next) { + if (ts && ref->type ==
>         REF_COMPONENT + &&
>         > ref->u.c.component->ts.type == BT_CLASS + && ref->next &&
>         > ref->next->type == REF_COMPONENT + && strcmp
>         > (ref->next->u.c.component->name, "_data") == 0 + &&
>         ref->next->next +
>         > && ref->next->next->type == REF_ARRAY + &&
>         ref->next->next->u.ar.type
>         > != AR_ELEMENT) + { + *ts = &ref->u.c.component->ts; +
>         class_ref = ref;
>         > + break; + } + + if (ts && *ts == NULL) + return NULL; +
>         Namely, if there is:
>            type1%array_class2 → array_class2 is used for 'ts' and
>         later (ok)
>            type1%type%array_class2 → NULL is returned  (why?)
>            class1%type%array_class2 → ts = class1 but array2_class is
>         used later on (ups!)
>            class1%...%scalar_class2 → ts = class1 but scalar_class2 is
>         used
>         etc.
>
>         Thus this either needs to be cleaned up (separate 'ref' loop for
>         ts != NULL) – including the wording in the description which
>         tells what
>         happens if 'ts' is passed as arg but the expr has rank == 0 – and
>         what value is assigned to 'ts'. (You can then also fix
>         'class.c::' to
>         'class.c: ' in the description above the function.)
>
>         Alternatively, you can leave the current code ref handling
>         code in place
>         at build_class_array_ref, which might be the simpler alternative.
>
>         Otherwise, it looks sensible to me.
>
>         Tobias
>
>         -----------------
>         Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634
>         München Registergericht München HRB 106955, Geschäftsführer:
>         Thomas Heurung, Frank Thürauf
>
>
>
>     --
>     "If you can't explain it simply, you don't understand it well
>     enough" - Albert Einstein
>
>
>
> --
> "If you can't explain it simply, you don't understand it well enough"
> - Albert Einstein
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank Thürauf