This is the mail archive of the
mailing list for the GCC project.
Re: PR 60414: Patch proposal
- From: Mikael Morin <mikael dot morin at sfr dot fr>
- To: Andre Vehreschild <vehre at gmx dot de>
- Cc: fortran at gcc dot gnu dot org, fxcoudert at gmail dot com, gcc-patches at gcc dot gnu dot org, Dominique d'Humières <dominiq at lps dot ens dot fr>
- Date: Sat, 26 Jul 2014 21:20:42 +0200
- Subject: Re: PR 60414: Patch proposal
- Authentication-results: sourceware.org; auth=none
- Authentication-results: sfrmc.priv.atos.fr; dkim=none (no signature); dkim-adsp=none (no policy) header dot from=mikael dot morin at sfr dot fr
- References: <20140721072605 dot 0D680105 at mailhost dot lps dot ens dot fr> <20140721150350 dot 10b35dd3 at vepi2 dot private> <911AF20A-F5E2-474F-858A-BF5CE56235D5 at lps dot ens dot fr>
Hello, thanks for your contribution.
here are some comments about the patch:
Le 21/07/2014 15:03, Andre Vehreschild a écrit :
> diff --git a/gcc/fortran/ChangeLog b/gcc/fortran/ChangeLog
> index c33936b..cb01a13 100644
> --- a/gcc/fortran/ChangeLog
> +++ b/gcc/fortran/ChangeLog
Changelogs are preferably provided outside of the patch as they almost
always conflict when applying it.
> @@ -1,3 +1,11 @@
> +2014-07-19 Andre Vehreschild <firstname.lastname@example.org>
> + PR fortran/60414
> + * interface.c (compare_parameter): Fix compile bug: During resolution
> + of generic an array reference in the actual argument was not
> + respected. Fixed by checking, if the ref member is non-null.
You don't need to explain why (in the Changelog I mean), only _what_ is
changed should be there.
> + unlimited_polymorphism_18.f90 add.
No need for that here; it's redundant with the testsuite Changelog.
> 2014-06-15 Tobias Burnus <email@example.com>
> * symbol.c (check_conflict): Add codimension conflict with
> diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
> index b210d18..d84c888 100644
> --- a/gcc/fortran/interface.c
> +++ b/gcc/fortran/interface.c
> @@ -2156,7 +2156,11 @@ compare_parameter (gfc_symbol *formal, gfc_expr
> if (symbol_rank (formal) == actual->rank || symbol_rank (formal) == -1)
> return 1;
> + /* Only check ranks compatibility, when the actual argument is not a
> + reference of an array (foo(i)). A reference into an array is assumed
> + when actual->ref is non null. */
> if (actual->ts.type == BT_CLASS && CLASS_DATA (actual)->as
> + && !actual->ref
> && CLASS_DATA (actual)->as->rank == symbol_rank (formal))
> return 1;
I think you have spotted the right place where the bug happens, but the
patch provided is not completely right for the following reason:
actual->ref points to the beginning of the reference chain, so
for example if actual is the gfc_expr struct for "var%comp%vec",
actual->ref points to the gfc_ref struct for "comp".
Furthermore, you have to be aware that even bare array variables
without sub-reference get an implicit full array reference in the AST, so
if actual is the gfc_expr struct for "array_var", actual->ref is
non-null and points (indirectly) to a gfc_array_ref of type AR_FULL.
So if you want to check for the absence of trailing sub-reference, you
have to walk down to the last reference chain.
But then you have to also support the case "var%array_comp%vec(1)" which
is supposed to have the rank of array_comp.
In fact I think the conditional supposed to support the CLASS cases is
completely bogus, and I don't see why the generic conditional just above
it wouldn't also support CLASS cases just fine.
Did you try removing the CLASS conditional entirely?