[Patch, Fortran, 4.6] Coarray 6/n: Add expression support

[Tobias Burnus]

Daniel Kraft wrote:
>> here comes the next coarray patch, which finally brings support for
>> coarrays in expression.
>> OK for the 4.6 trunk?
>
> Yes, but as usual see some minor comments below:
>
> +              gcc_assert (ref->u.ar.as->corank > 0);
> +          if (init == NULL)
>
> It seems the indentation is messed up (or inconsistent tab vs spaces)
> as both lines should be at the same level.
Fixed.

> +      m = gfc_match_array_spec (&as, true, false);
> +          current_as->rank = as->rank;
> +          current_as->type = as->type;
> +          current_as->cray_pointee = as->cray_pointee;
> +          current_as->cp_was_assumed = as->cp_was_assumed;
> [...]

> Is there a way to merge this with the similar block just before it in
> the patch (i.e., share between variable_decl and match_attr_spec)?  If
> so I'd appreciate not duplicating the code (but of course there may
> not be an easy way).
Done - though it is not as beautify as hoped for.

> +  if (last && last->u.c.component->ts.type == BT_CLASS)
> +    return
> last->u.c.component->ts.u.derived->components->attr.alloc_comp;
> +  else if (last && last->u.c.component->ts.type == BT_DERIVED)
> +    return last->u.c.component->ts.u.derived->attr.alloc_comp;
>
> Seeing this -- although not related to your patch directly --, I think
> it may be a good idea to abstract attribute checks like there, no?  So
> that not everywhere it must be known how the CLASS dummy struct is
> organized and that there is a check on CLASS / not CLASS needed.
I agree - and defer it someone else ;-)

> Just below is another such block.
>
> +/* Check whether the expression has an pointer component.
> +   Being itself a pointer does not count.  */
> +bool
> +gfc_has_ultimate_pointer (gfc_expr *e)
>
> I'm not sure how practicable that is, but once again this with the
> ultimate-allocatable check seems like code duplication to me.
> Unfortunatly I see no easy way to "flexibly" pass to a common routine
> which struct field (pointer_comp or alloc_comp) to read.  Having
> lambda-expressions or closures would be handy here :)

I left it as is - despite the code duplication; at least the code is
rather small and readable.

> The line seems to be longer than 80 characters
Fixed.

> +  if (expr->value.function.isym && expr->value.function.isym->inquiry)
> +    inquiry_argument = true;
>    no_formal_args = sym && is_external_proc (sym) && sym->formal == NULL;
> +
>    if (resolve_actual_arglist (expr->value.function.actual,
>                    p, no_formal_args) == FAILURE)
>        return FAILURE;
>
> +  inquiry_argument = false;
>
> Why don't you have to save & restore the old value of inquiry_argument
> here?  And is it intended that the value may stay true if the return
> FAILURE; is done?

Second question: It was an oversight and thus I have corrected it.

To the first question: C617 only applies to data-refs (i.e. variables)
and thus it is not applicable for functions (such as nested functions).
I have now added a comment that inquiry_argument is only for variables.

> +    case DIMEN_STAR:
> +    /* Check only the lower bound as the upper one is '*'.  */
>
> Makes sense, but to me the following code looks as if *both* upper and
> lower bound are checked.
I have removed the comment. The bounds check deals with * (which is
simply expr == NULL) and indeed both bounds are checked.

> +          c = ref2 ? ref2->u.c.component :
> e->symtree->n.sym->components;
> +      for ( ; c; c = c->next)
> As above, seems to be some indentation or tabs inconsistency.
Fixed.

> +  int i, pointer, allocatable, dimension, check_intent_in, is_abstract,
> +      coindexed = false;
>
> Probably nit-picky, but if coindexed is int here (instead of bool), I
> think you should better use 0 instead of false (or maybe FALSE).
Better nitpick by the current trunk: Set but not used variable:
coindexed. Thus I have removed it.

> +        coindexed = coindexed ? true : ref->u.ar.codimen > 0;
> What about
> if (ref->u.ar.codimen > 0)
>   coindexed = true;
Better - but no longer used (see above).

> Line longer than 80 characters.
Fixed.

> BTW, I suppose you're going to add the -fcoarrays=none checks here
> afterwards or something like that?
I have now added a check.

Attached you find the interdiff.

Thanks a lot for the review!

Tobias

Sending        gcc/fortran/ChangeLog
Sending        gcc/fortran/array.c
Sending        gcc/fortran/data.c
Sending        gcc/fortran/decl.c
Sending        gcc/fortran/expr.c
Sending        gcc/fortran/gfortran.h
Sending        gcc/fortran/interface.c
Sending        gcc/fortran/match.c
Sending        gcc/fortran/match.h
Sending        gcc/fortran/primary.c
Sending        gcc/fortran/resolve.c
Sending        gcc/fortran/trans-array.c
Sending        gcc/fortran/trans-expr.c
Sending        gcc/testsuite/ChangeLog
Adding         gcc/testsuite/gfortran.dg/coarray_7.f90
Adding         gcc/testsuite/gfortran.dg/coarray_8.f90
Transmitting file data ................
Committed revision 158149.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: interdiff.diff
Type: text/x-patch
Size: 11337 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20100409/7f3428be/attachment.bin>