Hi Andre,
Thanks for the review - I'll act on the points that you raised.
The Linaro people reported a failure in reduce_1.f90 execution, which I
believe is due to incorrect casting of 'dim' and a wrong specification of
its kind. I am waiting to hear back from them as to whether or not I have
fixed the failure.
Cheers
Paul
On Wed, 19 Mar 2025 at 12:39, Andre Vehreschild <ve...@gmx.de> wrote:
> Hi Paul,
>
> I took a look at your patch and think I found some improvements needed. In
>
> +bool
> +gfc_check_reduce (gfc_expr *array, gfc_expr *operation, gfc_expr *dim,
> + gfc_expr *mask, gfc_expr *identity, gfc_expr *ordered)
> +{
>
> ...
>
> + if (formal->sym->attr.allocatable || formal->sym->attr.allocatable
> + || formal->sym->attr.pointer || formal->sym->attr.pointer
> + || formal->sym->attr.optional || formal->sym->attr.optional
> + || formal->sym->ts.type == BT_CLASS || formal->sym->ts.type ==
> BT_CLASS)
> + {
> + gfc_error ("Each argument of OPERATION at %L shall be a scalar, "
> + "non-allocatable, non-pointer, non-polymorphic and "
> + "nonoptional", &operation->where);
> + return false;
> + }
>
> The if is only looking at the first formal argument. The right-hand sides
> of the || miss a ->next-> to look at the second formal argument, right?
>
> May be you also want to extend the tests!?
>
> Without having looked at it, but can't you extract the whole block of
>
> + if (array->ts.type == BT_CHARACTER)
> + {
> + unsigned long actual_size, formal_size1, formal_size2, result_size;
> ...
> + return false;
> + }
> + }
>
> and share it with the checks for co_reduce? I figure way to many DRY
> principle
> violations are in gfortran. So when we can start this, why not do it? And a
> call to a routine, like check_char_arg_conformance() speaks way better,
> then
> having to read all that code ;-)
>
> In gfc_resolve_reduce() identity and ordered are marked as UNUSED. Should
> these
> not a least be resolved?
>
> Please run contrib/check_GNU_style on your patch. It reports several issues
> (haven't look into their validity).
>
> In the Changelog:
>
> - (gfc_check_rename): Add prototype for intrinsic with 6 arguments.
> + * gfortran.h: Add prototype for intrinsic with 6 arguments.
>
> s/discription/description/
>
> I also encountered that nit with the executable stack when working in
> OpenCoarrays, but haven't had time (or desire) to look into it. I will put
> myself into CC of the pr Jerry mentioned.
>
> Besides the mentions above, this looks good to me.
>
> Thanks for the patch and
>
> Regards,
> Andre
>
>
>
> On Sun, 16 Mar 2025 17:26:55 +0000
> Paul Richard Thomas <paul.richard.tho...@gmail.com> wrote:
>
> > Hi All,
> >
> > This version of the REDUCE intrinsic patch has evolved somewhat since the
> > posting on 2nd March. The most important changes are to the wrapper
> > function and the addition of two testsuite entries.
> >
> > The wrapper function now effects:
> > subroutine wrapper (a, b, c)
> > type_of_ARRAY, intent(inout) :: a, c
> > type_of_ARRAY, intent(inout), optional :: b
> > if (present (b)) then
> > c = OPERATION (a,b )
> > else
> > c = a
> > end if
> > end subroutine
> >
> > The reason for wrapping OPERATION in a subroutine is to allow pointer
> > arithmetic to be used throughout in the library function. The only thing
> > that needs to be known about the type and kind of ARRAY is the element
> > size. The second branch in the wrapper allows deep copies to be done in
> the
> > library function, such that derived types with allocatable components do
> > not leak memory. This is needed at the final step of the algorithm to
> copy
> > the result from each iteration to the result and then nullify it.
> >
> > This is undoubtedly a bit heavy going for intrinsic types and so, one day
> > soon I will possibly do a bit of M4ery. That said, the present version
> > works for all types of ARRAY and I worry a bit about how much this
> > intrinsic will be used. Thoughts?
> >
> > A slight niggle is the linker error that comes up if compiled without any
> > optimization:
> > /usr/bin/ld: warning: /tmp/cc9cx9Rw.o: requires executable stack (because
> > the .note.GNU-stack section is executable)
> > I think that this is unlikely to present a security issue, however, since
> > it disappears at -O1, I went through each of the options triggered by -O1
> > but couldn't make it go away. Does anybody know why this is?
> >
> > Regtests OK with FC41/x86_64 - OK for mainline?
> >
> > Regards
> >
> > Paul
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
>
