Re: [Patch, fortran] PR59104

[Harald Anlauf <anlauf@gmx.de>]

Hi Paul,

this looks good to me and is OK for mainline.  When it has survived a
week or two, backporting at least to 14-branch (ideally before 14.2
release) would be a good thing!

Regarding the following excerpt of the testcase:

+! Commented out lines give implicit type warnings with gfortran and nagfor
+!        character(len = len (d)) :: line4 (len (line3))
+        character(len = len (line3)) :: line4 (len (line3))
+!        character(len = size(len4, 1)) :: line5

I guess the last commented line should have referred to size(line4, 1),
right?  The lexical distance of len4 and line4 can be small after
long coding...

The first commented line gives no warning here, but is simply
inconsistent with a test later on, since len (d) < len (line3).
What exactly was the issue?

***

A minor nit: while you were fixing whitespace issues in the source,
you missed an indent with spaces here:

@@ -857,12 +873,26 @@ gfc_defer_symbol_init (gfc_symbol * sym)
        /* Find the first dummy arg seen after us, or the first
non-dummy arg.
           This is a circular list, so don't go past the head.  */
        while (p != head
-             && (!p->attr.dummy || p->dummy_order > sym->dummy_order))
+             && (!p->attr.dummy || decl_order (p, sym)))
          {

At least on my side there is no tab...
(It is fine in a similar code later on.)

***

Finally a big thanks for the patch!

Harald


Am 13.06.24 um 23:43 schrieb Paul Richard Thomas:
> Hi Both,
>
> Thanks for the highly constructive comments. I think that I have
> incorporated them fully in the attached.
>
> OK for mainline and ...?
>
> Paul
>
>
> On Mon, 10 Jun 2024 at 08:19, Andre Vehreschild <vehre@gmx.de> wrote:
>
>> Hi Paul,
>>
>> while looking at your patch I see calls to gfc_add_init_cleanup (...,
>> back),
>> while the function signature is gfc_add_init_cleanup (..., bool front).
>> This
>> slightly confuses me. I would at least expect to see
>> gfc_add_init_cleanup(...,
>> !back) calls. Just to get the semantics right.
>>
>> Then I wonder why not doing:
>>
>> diff --git a/gcc/fortran/dependency.cc b/gcc/fortran/dependency.cc
>> index bafe8cbc5bc..97ace8c778e 100644
>> --- a/gcc/fortran/dependency.cc
>> +++ b/gcc/fortran/dependency.cc
>> @@ -2497,3 +2497,63 @@ gfc_omp_expr_prefix_same (gfc_expr *lexpr, gfc_expr
>> *rexpr)
>>     return true;
>>   }
>> +
>> +
>> +/* gfc_function_dependency returns true for non-dummy symbols with
>> dependencies
>> +   on an old-fashioned function result (ie. proc_name =
>> proc_name->result).
>> +   This is used to ensure that initialization code appears after the
>> function
>> +   result is treated and that any mutual dependencies between these
>> symbols are
>> +   respected.  */
>> +
>> +static bool
>> +dependency_fcn (gfc_expr *e, gfc_symbol *sym,
>> +                int *f ATTRIBUTE_UNUSED)
>> +{
>> +  return (e && e->expr_type == EXPR_VARIABLE
>> +      && e->symtree
>> +      && e->symtree->n.sym == sym);
>> +}
>>
>> Instead of the multiple if-statements?
>>
>> +
>> +bool
>> +gfc_function_dependency (gfc_symbol *sym, gfc_symbol *proc_name)
>> +{
>> +  bool front = false;
>> +
>> +  if (proc_name && proc_name->attr.function
>> +      && proc_name == proc_name->result
>> +      && !(sym->attr.dummy || sym->attr.result))
>> +    {
>> +      if (sym->as && sym->as->type == AS_EXPLICIT)
>> +       {
>> +         for (int dim = 0; dim < sym->as->rank; dim++)
>> +           {
>> +             if (sym->as->lower[dim]
>> +                 && sym->as->lower[dim]->expr_type != EXPR_CONSTANT)
>> +               front = gfc_traverse_expr (sym->as->lower[dim], proc_name,
>> +                                          dependency_fcn, 0);
>> +             if (front)
>> +               break;
>> +             if (sym->as->upper[dim]
>> +                 && sym->as->upper[dim]->expr_type != EXPR_CONSTANT)
>> +               front = gfc_traverse_expr (sym->as->upper[dim], proc_name,
>> +                                          dependency_fcn, 0);
>> +             if (front)
>> +               break;
>> +           }
>> +       }
>> +
>> +      if (sym->ts.type == BT_CHARACTER
>> +         && sym->ts.u.cl && sym->ts.u.cl->length
>> +         && sym->ts.u.cl->length->expr_type != EXPR_CONSTANT)
>> +       front = gfc_traverse_expr (sym->ts.u.cl->length, proc_name,
>> +                                  dependency_fcn, 0);
>>
>> This can overwrite a previous front == true, right? Is this intended?
>>
>> +    }
>> +  return front;
>> + }
>>
>> The rest - besides the front-back confusion - looks fine to me. Thanks for
>> the
>> patch.
>>
>> Regards,
>>          Andre
>>
>> On Sun, 9 Jun 2024 07:14:39 +0100
>> Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote:
>>
>>> Hi All,
>>>
>>> The attached fixes a problem that, judging by the comments, has been
>> looked
>>> at periodically over the last ten years but just looked to be too
>>> fiendishly complicated to fix. This is not in small part because of the
>>> confusing ordering of dummies in the tlink chain and the unintuitive
>>> placement of all deferred initializations to the front of the init chain
>> in
>>> the wrapped block.
>>>
>>> The result of the existing ordering is that the initialization code for
>>> non-dummy variables that depends on the function result occurs before any
>>> initialization code for the function result itself. The fix ensures that:
>>> (i) These variables are placed correctly in the tlink chain, respecting
>>> inter-dependencies; and (ii) The dependent initializations are placed at
>>> the end of the wrapped block init chain.  The details appear in the
>>> comments in the patch. It is entirely possible that a less clunky fix
>>> exists but I failed to find it.
>>>
>>> OK for mainline?
>>>
>>> Regards
>>>
>>> Paul
>>
>>
>> --
>> Andre Vehreschild * Email: vehre ad gmx dot de
>>
>