Hi Alessandro,
thanks for the patch. Some polishing is still necessary:
Running in the source directory of gcc:
contrib/check_GNU_style.sh resurrected_patch_and_tests_REV1.diff
gives about 10 issues. Please correct them before applying. Style in gfortran
helps readability.
In check.c::gfc_check_image_status () you are checking the kind of the image's
argument to be gt gfc_default_integer_kind and lt twice the default. Why? In
the standard I see no argument to limit the kind of the arguments. Can you
elaborate?
In the same routine: All operators are standing alone, i.e. put space before
and after each operator (e.g. line 32: gfc_default_integer_kind*2 should be
'..._kind * 2'
You are introducing the notion of teams here in the error messages, but the
rest of gfortran does not have any knowledge about teams. This might confuse
users even if is saying that team is not supported. Just as a remark.
In intrinsic.c you declare the symbol "failed_images" l. 196 as
CLASS_TRANSFORMATIONAL. What data does the statement transform in which way? I
think CLASS_INQUIRY would be better suited, because the function is just asking
the runtime for information.
Same for "image_status" l. 209 and "stopped_images" l. 221.
In line 511 I feel like returning NULL in caf_single mode is insufficient.
Imagine an assignment f = failed_images(). Returning NULL will most likely make
the compiler ICE, when evaluating the rhs (haven't tested though). Returning a
constant 0 expression would be more wise, because in caf_single mode only the
current image is present and that must be running to do the inquiry.
Same for the stopped images in l. 538.
and image_status in l. 563.
The FIXME in line 566 needs to be resolved or one of the middle-end guys will
step on your toes, when that fails.
What are the three arguments to caf_failed_images in line 610. Most
interestingly the first one?
And in line 644 you see the result of returning NULL in the simplify_()
routines. Please remove this again here and return something reasonable in the
simplify-routines as suggested above. Checking for arg-expr not being
initialized here might lead to hard to find misbehavior of gfortran in other
cases.
Line 689 and 695: Do not sort symbols as a side-effect of a functional patch.
Correct style and change sort orders and the like in a separate patch for code,
that is not intrinsic to what you patch. It makes reviewers wonder why you
needed to change that!
Line 717: Why is a block needed here? You may just return the call and be done.
Line 725: Would it not be better to call the numeric stop function? With a
documented error code?
Line 783: As Jerry has pointed out already: This needs to be dg-do compile.
Furthermore is the coarray directory the wrong place, because there all tests
are called ones with -fcoarray=single and ones with -fcoarray=lib -lcaf_single
-latomic. So the test needs to go to gfortran.dg name it e.g.,
coarray_fail_st_1.f90.
Just checking above that the code compiles is only a quarter of the way. You
still don't know, that correct API-calls are generated. This has to be added
for -fcoarray=single and -fcoarray=lib.
Line 798: Same for this test as for the previous: Wrong test-mode,
wrong directory, not checking API-calls correctly. Also add a number to the
test's file name. It most likely will not be the only test for that feature.
Line 819: Same as for the previous two.
Line 881: Make it:
! { dg-final { scan-tree-dump-times "_gfortran_caf_failed_images
\\\(&atmp\\\.\[0-9\]+, 0B, 0B\\\);" 1 "original" } }
Experience has shown, that gfortrans on different systems choose quite
arbitrary numbers for the atmp and then the test fails.
Line 989: Please remove the dependency on signal.h. I don't assume it is
present on all systems and you don't want to do the guard thing.
Line 999: Use exit(1); here instead of the sigkill. This would sync termination
with the way it is done by error_stop() and obsoletes the need for signal.h.
Overall: You are adding several API-functions without a single line of
documentation in gfortran.texi. This is not good.
Therefore my rating: NOT ok for trunk yet.
Regards,
Andre
On Mon, 13 Feb 2017 13:35:37 -0700
Alessandro Fanfarillo <fanfarillo....@gmail.com> wrote:
> Now with the patch attached.
>
> 2017-02-13 13:35 GMT-07:00 Alessandro Fanfarillo <fanfarillo....@gmail.com>:
> > Thanks Jerry. That test case is supposed only to be compiled (it never
> > runs). Anyway, the attached patch has been modified according to your
> > suggestion.
> >
> > Patch built and regtested on x86_64-pc-linux-gnu.
> >
> > 2017-02-12 10:24 GMT-07:00 Jerry DeLisle <jvdeli...@charter.net>:
> >> On 02/11/2017 03:02 PM, Alessandro Fanfarillo wrote:
> >>>
> >>> Dear all,
> >>> please find in attachment a new patch following the discussion at
> >>> https://gcc.gnu.org/ml/fortran/2017-01/msg00054.html.
> >>>
> >>> Suggestions on how to fix potential issues are more than welcome.
> >>>
> >>> Regards,
> >>> Alessandro
> >>>
> >>
> >> On the failed images test:
> >>
> >> program test_image_status
> >> + implicit none
> >> +
> >> + write(*,*) image_status(1)
> >> +
> >>
> >> Write to a string and test the results.
> >>
> >> I assume you have regression tested this again as stated in the earlier
> >> discussion.
> >>
> >> I think this is OK to go in.
> >>
> >> Jerry
> >>
> >>
--
Andre Vehreschild * Email: vehre ad gmx dot de
