This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC 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: [Patch, Fortran, 4.6] Coarray 6/n: Add expression support


Hi Tobias,

Tobias Burnus wrote:
Hi all,

here comes the next coarray patch, which finally brings support for
coarrays in expression. If one replaces "THIS_IMAGE()" by "1", most
coarray programs should compile now. That's the case for the most test
cases at http://users.physik.fu-berlin.de/~tburnus/coarray/ .
Sorry that the patch is a rather large.

after some while, I finally managed the review of this one ;) (BTW, I looked at your "updated" version sent in a follow-up mail of course.)


The patch was build and regtested on x86-64-linux.
OK for the 4.6 trunk?

Yes, but as usual see some minor comments below:


+	  if (ref->u.ar.as->rank == 0)
+	    {
+              gcc_assert (ref->u.ar.as->corank > 0);
+	      if (init == NULL)

It seems the indentation is messed up (or inconsistent tab vs spaces) as both lines should be at the same level.


+ m = gfc_match_array_spec (&as, true, false); + if (current_as == NULL) + current_as = as; + else if (m == MATCH_YES) + { + int i; + gcc_assert (current_as->rank == 0 && current_as->corank > 0 + && as->rank > 0 && as->corank == 0); + current_as->rank = as->rank; + current_as->type = as->type; + current_as->cray_pointee = as->cray_pointee; + current_as->cp_was_assumed = as->cp_was_assumed; + + for (i = 0; i < current_as->corank; i++) + { + current_as->lower[as->rank + i] = current_as->lower[i]; + current_as->upper[as->rank + i] = current_as->upper[i]; + } + for (i = 0; i < as->rank; i++) + { + current_as->lower[i] = as->lower[i]; + current_as->upper[i] = as->upper[i]; + } + + gfc_free (as); + }

Is there a way to merge this with the similar block just before it in the patch (i.e., share between variable_decl and match_attr_spec)? If so I'd appreciate not duplicating the code (but of course there may not be an easy way).

+  if (last && last->u.c.component->ts.type == BT_CLASS)
+    return last->u.c.component->ts.u.derived->components->attr.alloc_comp;
+  else if (last && last->u.c.component->ts.type == BT_DERIVED)
+    return last->u.c.component->ts.u.derived->attr.alloc_comp;

Seeing this -- although not related to your patch directly --, I think it may be a good idea to abstract attribute checks like there, no? So that not everywhere it must be known how the CLASS dummy struct is organized and that there is a check on CLASS / not CLASS needed.

Just below is another such block.

+/* Check whether the expression has an pointer component.
+   Being itself a pointer does not count.  */
+bool
+gfc_has_ultimate_pointer (gfc_expr *e)

I'm not sure how practicable that is, but once again this with the ultimate-allocatable check seems like code duplication to me. Unfortunatly I see no easy way to "flexibly" pass to a common routine which struct field (pointer_comp or alloc_comp) to read. Having lambda-expressions or closures would be handy here :)

+ gfc_error ("Invalid expression in the derived type constructor for pointer "
+ "component '%s' at %L in PURE procedure", comp->name,


The line seems to be longer than 80 characters (it wraps in my Vi for more than the additional +); please merge it with the follow-up one so that both are at most 80 characters long, should be no big deal.

+  if (expr->value.function.isym && expr->value.function.isym->inquiry)
+    inquiry_argument = true;
   no_formal_args = sym && is_external_proc (sym) && sym->formal == NULL;
+
   if (resolve_actual_arglist (expr->value.function.actual,
 			      p, no_formal_args) == FAILURE)
       return FAILURE;

+ inquiry_argument = false;

Why don't you have to save & restore the old value of inquiry_argument here? And is it intended that the value may stay true if the return FAILURE; is done?

+    case DIMEN_STAR:
+	/* Check only the lower bound as the upper one is '*'.  */

Makes sense, but to me the following code looks as if *both* upper and lower bound are checked. If the * is handled correctly by compare_bounds, please add at least some comment there to confirm or clear this.

+          c = ref2 ? ref2->u.c.component : e->symtree->n.sym->components;
+	  for ( ; c; c = c->next)

As above, seems to be some indentation or tabs inconsistency.

+  int i, pointer, allocatable, dimension, check_intent_in, is_abstract,
+      coindexed = false;

Probably nit-picky, but if coindexed is int here (instead of bool), I think you should better use 0 instead of false (or maybe FALSE).

+ coindexed = coindexed ? true : ref->u.ar.codimen > 0;

What about = coindexed || (ref->u.ar.codimen > 0)?
Or maybe even

if (ref->u.ar.codimen > 0)
  coindexed = true;

is clearer and simpler.

+ gfc_error ("Sorry, allocatable coarrays are no yet supported coarray at %L",

Line longer than 80 characters.


BTW, I suppose you're going to add the -fcoarrays=none checks here afterwards or something like that? Should also be ok.


Thanks a lot for your work here, I hope I can join you at some point!

Daniel

--
Done:  Arc-Bar-Cav-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Kni-Mon-Pri


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