This is the mail archive of the
fortran@gcc.gnu.org
mailing list for the GNU Fortran project.
Re: First Patch
- From: Tobias Burnus <tobias dot burnus at physik dot fu-berlin dot de>
- To: Daniel Carrera <dcarrera at gmail dot com>, fortran at gcc dot gnu dot org
- Date: Tue, 7 Jun 2011 16:51:06 +0200
- Subject: Re: First Patch
Hello Daniel,
Thanks for the patch. Some preliminary comments to your patch.
First, patches should also be also (CC) send to gcc-patches@.
(gfortraners are all subscribed to fortran@ - and many aren't to
gcc-patches@; however, sending them also to gcc-patches@ allows
non-Fortran developers to comment on Fortran changes.)
Secondly, I would prefer a more explicit email subject; semi-standard
seems to be "[Patch, Fortran]" followed by some descriptor and, if
applicable, a bug number (PR ...). It's not really needed, but more
convenient - especially if you want to find the initial patch email
months/years later. (The "Patch" part is to alert fortran@ readers
that there is patch to be reviewed, the "Fortran" part is to quickly
tell the gcc-patches@ readers that it is a Fortran patch.)
Thirdly, you miss a changelog entry. It should be not a diff (as
ChangeLog will likely change.) Note that the indention is via a
tab and that there are two spaces between date and name and between
name and email address. See gcc/fortran/ChangeLog and
gcc/testsuite/ChangeLog for examples. The changelog should contain
what has been changed - not why. Cf. also
http://www.gnu.org/prep/standards/html_node/Style-of-Change-Logs.html
Coding style: I think you miss several tabs: Coding style is to replace
spaces by a tab if possible (8 spaces = tab). Regarding:
+ casting it to the correct type.
+ */
The */ should be in the preceeding line, i.e. "... type. */" with two
spaces before the "*/".
+ if (stat == null_pointer_node || TREE_TYPE(stat) == integer_type_node)
+ tmp = build_call_expr_loc( input_location, gfor_fndecl_caf_sync_all,
Coding style is to place a space before the "(" and none after it, i.e.
"TREE_TYPE (stat)" and "..._expr_loc (input_...".
}
- else
+
+ else
{
The "else" looks wrongly indented.
+ 5, integer_type_node, integer_type_node, pint_type, build_pointer_type (pchar_type_node),
That line is too long. Additionally, you need a pointer to an integer
("int *") and not an integer ("int") as argument. Thus, you should use
"pint_type" instead of "integer_type_node".
As a matter of taste, I'd would prefer for SYNC IMAGES:
_gfortran_caf_sync_images (int count, int images[], int *status,
char *errmsg, int errmsg_len)
instead of
_gfortran_caf_sync_images (int *status, int count, int images[],
char *errmsg, int errmsg_len)
to be more in line with the standard ("SYNC IMAGES (imageset, stat-list)")
and to place optional values after the required ones.
+_gfortran_caf_sync_all (int *status, char *errmsg, int errmsg_len)
{
- int ierr;
- ierr = MPI_Barrier (MPI_COMM_WORLD);
+ status = MPI_Barrier (MPI_COMM_WORLD);
There are two issues:
a) You don't want to do a pointer assignment but you want to
assign the value, i.e.: *status = MPI_...
b) If the argument is absent - i.e. it is a NULL pointer -, one
cannot assign to the variable.
The easiest to probably to keep ierr - and later set stat via
if (state)
state = ierr;
Could you also change "status" into "stat"? I know that I used "status",
but I think it makes sense to follow the standard and use "stat".
The error diagnostic should be as follows:
- If no error occurs - and "stat" is not present, don't do anything
special
- If no error occurs - and "stat" is present (not NULL), assign 0 to it.
- If an error occurs - and "stat" is not present, print an error and exit
- If an error occurs - and "stat" is present, assign the error code to it
- and if "errmsg" is present, set it.
(One could also defer this part.)
Seemingly, we need a test case for SYNC ALL/IMAGES - with and without stat=
argument. For the argument version, one should set "stat" to some value and
check afterwards that it is indeed 0.
The test case has to be placed into gcc/testsuite/gfortran.dg/coarray/; if
it is in that directory, it is automatically tested with both
-fcoarray=single and with -fcoarray=lib. I think some test case
gcc/testsuite/gfortran.dg/coarray_*.f90 could be used as template. The test
has to be "! { dg-do run }" as the library has to be checked.
Tobias