Hi Paul,
in addition to Thomas' remarks (which I second to), I have the following:
> diff --git a/gcc/fortran/intrinsic.cc b/gcc/fortran/intrinsic.cc
> index 0a6be215825..d95f35145b5 100644
> --- a/gcc/fortran/intrinsic.cc
> +++ b/gcc/fortran/intrinsic.cc
> @@ -293,11 +293,15 @@ do_ts29113_check (gfc_intrinsic_sym *specific,
> gfc_actual_arglist *arg)
> &a->expr->where, gfc_current_intrinsic);
> ok = false;
> }
> - else if (a->expr->rank == -1 && !specific->inquiry)
> + else if (a->expr->rank == -1
> + && !(specific->inquiry
> + || (specific->id == GFC_ISYM_RESHAPE
> + && (gfc_option.allow_std & GFC_STD_F202Y))))
> {
> gfc_error ("Assumed-rank argument at %L is only permitted as actual "
> - "argument to intrinsic inquiry functions",
> - &a->expr->where);
> + "argument to intrinsic inquiry functions or to reshape. "
Is it not a convention to write Fortran intrinsics function names all
uppercase? I.e. RESHAPE when the function is meant just to make it clear like in
the message above on C_LOC and PRESENT (lines 268--270).
> + "The latter is an experimental F202y feature. Use "
> + "-std=f202y to enable", &a->expr->where);
> ok = false;
> }
> else if (a->expr->rank == -1 && arg != a)
> @@ -307,6 +311,13 @@ do_ts29113_check (gfc_intrinsic_sym *specific,
> gfc_actual_arglist *arg) &a->expr->where, gfc_current_intrinsic);
> ok = false;
> }
> + else if (a->expr->rank == -1 && specific->id == GFC_ISYM_RESHAPE
> + && !gfc_is_simply_contiguous (a->expr, true, false))
> + {
> + gfc_error ("Assumed rank argument to the reshape intrinsic at %L "
Here, too?
> + "must be contiguous", &a->expr->where);
> + ok = false;
> + }
> }
>
> return ok;
<snipp>
> diff --git a/gcc/fortran/match.cc b/gcc/fortran/match.cc
> index 0cd78a57a2f..81610b93345 100644
> --- a/gcc/fortran/match.cc
> +++ b/gcc/fortran/match.cc
> @@ -1920,7 +1920,31 @@ gfc_match_associate (void)
> gfc_association_list* a;
>
> /* Match the next association. */
> - if (gfc_match (" %n =>", newAssoc->name) != MATCH_YES)
> + if (gfc_match (" %n ", newAssoc->name) != MATCH_YES)
> + {
> + /* "Expected associate name at %C" would be better.
> + Change associate_3.f03 to match. */
That's an odd comment. Sounds to me like a remark to your self.
> + gfc_error ("Expected associate name at %C");
> + goto assocListError;
> + }
> +
> + /* Required for an assumed rank target. */
> + if (gfc_peek_char () == '(')
> + {
> + newAssoc->ar = gfc_get_array_ref ();
This is not freeed in case of an error and may result in a memory leak, right?
> + if (gfc_match_array_ref (newAssoc->ar, NULL, 0, 0) != MATCH_YES)
> + {
> + gfc_error ("Bad bounds remapping list at %C");
> + goto assocListError;
> + }
> + }
> +
> + if (newAssoc->ar && !(gfc_option.allow_std & GFC_STD_F202Y))
> + gfc_error_now ("The bounds remapping list at %C is an experimental "
> + "F202y feature. Use std=f202y to enable");
> +
> + /* Match the next association. */
> + if (gfc_match (" =>", newAssoc->name) != MATCH_YES)
> {
> gfc_error ("Expected association at %C");
> goto assocListError;
<snipp>
> diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> index 07e28a9f7a8..aa0ee1b0164 100644
> --- a/gcc/fortran/trans-expr.cc
> +++ b/gcc/fortran/trans-expr.cc
<snipp>
> @@ -10784,6 +10815,13 @@ gfc_trans_pointer_assignment (gfc_expr * expr1,
> gfc_expr * expr2)
> gcc_assert (remap->u.ar.start[dim] &&
> remap->u.ar.end[dim]);
> + if (remap->u.ar.start[dim]->expr_type != EXPR_CONSTANT
> + || remap->u.ar.start[dim]->expr_type != EXPR_VARIABLE)
> + gfc_resolve_expr (remap->u.ar.start[dim]);
> + if (remap->u.ar.end[dim]->expr_type != EXPR_CONSTANT
> + || remap->u.ar.end[dim]->expr_type != EXPR_VARIABLE)
> + gfc_resolve_expr (remap->u.ar.end[dim]);
> +
Can't these resolves be done during resolve-stage? I have had some serious
trouble with late resolves, therefore asking.
> /* Convert declared bounds. */
> gfc_init_se (&lower_se, NULL);
> gfc_init_se (&upper_se, NULL);
<snipp>
> diff --git a/gcc/fortran/trans-stmt.cc b/gcc/fortran/trans-stmt.cc
> index 86c54970475..450c11c06d7 100644
> --- a/gcc/fortran/trans-stmt.cc
> +++ b/gcc/fortran/trans-stmt.cc
> @@ -1910,6 +1910,20 @@ trans_associate_var (gfc_symbol *sym,
> gfc_wrapped_block *block) gfc_add_init_cleanup (block, gfc_finish_block
> (&se.pre), tmp); }
> /* Now all the other kinds of associate variable. */
> + else if (e->rank == -1 && sym->attr.pointer && sym->assoc->ar)
> + {
> + gfc_expr *expr1 = gfc_lval_expr_from_sym (sym);
> + gfc_free_ref_list (expr1->ref);
What if sym.ts.type == BT_CLASS? I doubt this works in this case...
> + expr1->ref = gfc_get_ref();
> + expr1->ref->type = REF_ARRAY;
> + expr1->ref->u.ar = *sym->assoc->ar;
> + expr1->ref->u.ar.type = AR_SECTION;
> + gfc_expr *expr2 = gfc_copy_expr (e);
> + tmp = gfc_trans_pointer_assignment (expr1, expr2);
> + gfc_add_init_cleanup (block, tmp, NULL);
> + gfc_free_expr (expr1);
> + gfc_free_expr (expr2);
> + }
> else if ((sym->attr.dimension || sym->attr.codimension) && !class_target
> && (sym->as->type == AS_DEFERRED || sym->assoc->variable))
> {
<snipp>
> diff --git a/gcc/testsuite/gfortran.dg/f202y/f202y.exp
> b/gcc/testsuite/gfortran.dg/f202y/f202y.exp
> new file mode 100644
> index 00000000000..737a78937a7
> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/f202y/f202y.exp
<snipp>
> +global gfortran_test_path
> +global gfortran_aux_module_flags
> +set gfortran_test_path $srcdir/$subdir
> +set gfortran_aux_module_flags "-Werror -std=f2023"
I would have bet, that it should be -std=f202y ...
I mean, the directory says so. I get why you don't do it, but it is soooo
counter-intuitive to me. I would put my f202y tests in here and would then be
wondering why they don't work out of the box.
<snipp>
I hope those comments help at least a little bit. When you addressed them (in
which way ever), the patch is ok for mainline from my side.
Regards and thanks for the work,
Andre
On Mon, 23 Sep 2024 16:15:05 +0200
Thomas Koenig <tkoe...@netcologne.de> wrote:
> Am 23.09.24 um 11:02 schrieb Paul Richard Thomas:
> > Hi All,
> >
> > The moment I saw the DIN4 proposal for "Generic processing of assumed
> > rank objects", I thought that this was a highly intuitive and
> > implementable proposal. I implemented a test version in June and had
> > some correspondence with Reinhold Bader about it shortly before he
> > passed away.
> >
> > Malcolm Cohen wrote J3/24-136r1 in response to this and I have posted a
> > comment in PR116733 addressing the the extent to which the attached
> > patch addresses his remarks.
>
> I think your approaches are sound.
>
> > Before this patch goes through the approval process, we have to consider
> > how experimental F202y features can be carried forward. I was badly
> > bitten by failing to synchronise the array descriptor reform branch to
> > the extent that I gave up on it and adopted the simplified reform that
> > is now in place. Given the likely timescale before the full adoption of
> > the F202y standard, this is a considerable risk for
> > experimental features, given the variability of active maintainers:
> >
>
> That is correct. We (well, Nicolas) also saw the bit rot on the
> native coarray branch.
>
> > What I propose is the following:
> > (i) For audit purposes, I have opened PR116732, which should be blocked
> > by PRs for each experimental F202y feature;
>
> I have added PR 116025 to this.
>
> > (ii) These PRs should represent a complete audit trail for each feature; and
> > (iii) All such experimental features should be enabled on mainline by
> > --std=f202y, which is equivalent to -std=f2023+f202y.
>
> As far as the -funsigned patch goes: I would like to keep the option
> itself, but also enable it with -std=f202y.
>
> > The attached patch enables pointer assignment and associate, both with
> > rank remapping, plus the reshape intrinsics. which was not part of the
> > DIN4 proposal.
> >
> > The ChangeLog entries do a pretty complete job of describing the patch.
> >
> > Regtests correctly. OK for mainline?
>
> As a general remark: What we currently have are extensions, and we
> should try to describe them so a user who is not familiar with the
> J3 documents or our PRs and commit messages should be able to use
> the features. I am certainly not there yet with the work on UNSIGNED,
> but we should work on that so the documentation is fairly complete
> when gcc15 is released.
>
> As for the patch itself: It looks good do me in principle. It still
> needs some test cases (or did git omit these from your patch?
> I've been bitten by that :-). One typo:
>
> + gfc_error ("The data-target at %L ia an assumed rank object and
> so the "
>
> s/ia/is/
>
> So, OK in principle with reasonable test coverage, but if you could hold
> for a few days before committing so others can also comment, that would
> be good.
>
> Best regards
>
> Thomas
>
--
Andre Vehreschild * Email: vehre ad gmx dot de
