Re: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000]

[Harald Anlauf <anlauf@gmx.de>]

Hi Mikael,

> Gesendet: Donnerstag, 06. Oktober 2022 um 22:14 Uhr
> Von: "Mikael Morin" <morin-mikael@orange.fr>
> An: "Harald Anlauf" <anlauf@gmx.de>
> Cc: "fortran" <fortran@gcc.gnu.org>, "gcc-patches" <gcc-patches@gcc.gnu.org>
> Betreff: Re: [PATCH, v2] Fortran: error recovery for invalid types in array constructors [PR107000]
>
> Le 05/10/2022 à 23:40, Harald Anlauf a écrit :
> > 
> >> There is one last thing that I'm dissatisfied with.
> >> The handling of unknown types should be moved to reduce_binary, because
> >> the dispatching in reduce_binary doesn't handle EXPR_OP, so even if
> >> either or both operands are scalar, they are handled by the (array vs
> >> array) reduce_binary_aa function.  That's confusing.
> 
> Thinking about it again, I'm not sure my suggestion is right here.
> > 
> > Do you have an example?
> > 
> No.  Actually, I think it works, but a weird way.
> 
> 
> For example, for this case:
> 
> [real :: 2] * [real :: +(.true.)]
> 
> First there is a "root" invocation of reduce binary with arguments [real 
> :: 2] and [real :: +(.true.)]
> The root invocation of reduce_binary will call reduce_binary_aa. This is 
> normal.
> 
> Then reduce_binary_aa calls reduce_binary again with arguments 2 and 
> +(.true.).  And reduce_binary calls again reduce_binary_aa with those 
> arguments.  This is weird, reduce_binary_aa is supposed to have arrays 
> for both arguments.

Am I seeing something different from you?  My gdb says
that one argument of reduce_binary is EXPR_CONSTANT,
the other EXPR_OP and BT_UNKNOWN.  Both rank 0.

> The same goes for the array vs constant case, reduce_binary_ca (or 
> reduce_binary_ac) is invoked with two scalars, while if you look at 
> reduce_binary, you would expect that we only get to reduce_binary_ca 
> with a scalar constant and an array as arguments.
> 
> 
> I think the checks in the three reduce_binary_* functions should be 
> moved into their respective loops, so that we detect the invalid type 
> just before these weird recursive calls instead of just after entering 
> into them.

I think I tried that before, and it didn't work.
There was always one weird case that lead to a bad or
invalid constructor for one of the arrays you want to
look at in the respective loop,  and this is why the
testcase tries to cover everything that I hit then and
there... (hopefully).  So I ended up with the check
before the loop.

What do we actually gain with your suggested change?
Moving the check into the loop does not really make
the code more readable to me.  And the recursion is
needed anyway.

Cheers,
Harald

> OK with that change.
>