This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]