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

[Paul Richard Thomas <paul.richard.thomas@gmail.com>]

Pushed to master in commit 9a0e09f3dd5339bb18cc47317f2298d9157ced29

Thanks

Paul


On Wed, 14 Apr 2021 at 14:51, Tobias Burnus <tobias@codesourcery.com> wrote:

> 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
>


-- 
"If you can't explain it simply, you don't understand it well enough" -
Albert Einstein