Re: [Patch, Fortran, 4.6] F2008 - first coarray patch

[Daniel Kraft <d at domob dot eu>]

Hi,

Tobias Burnus wrote:
> OK for the 4.6 trunk?

Yes, but please consider my comments below.

What are your next plans here?  It seems to me that the next step (for 
single-image support) would be arrays with co-rank, right?

+    case EXEC_SYNC_ALL:
+      fputs ("SYNC ALL ", dumpfile);
+      if (c->expr2 != NULL)
+        {
+	  fputs (" stat=", dumpfile);
+	  show_expr (c->expr2);
+	}
+      if (c->expr3 != NULL)
+        {
+	  fputs (" errmsg=", dumpfile);
+	  show_expr (c->expr3);
+	}
+      break;

(and following)  I'm not sure what happened here, but it looks as if the 
indentation is screwed up (for matching {} pairs).

+  if ((code->expr1 && gfc_option.rtcheck & GFC_RTCHECK_BOUNDS) || 
code->expr2)

(again similar stuff following)  This is once again a minor comment, but 
I'd personally prefer to have () around the & operation in this case.

   if (bitmap_bit_p (cs_base->reachable_labels, label->value))
-    return;
+    {
+      for (stack = cs_base; stack; stack = stack->prev)
+	if (stack->current->op == EXEC_CRITICAL
+	    && bitmap_bit_p (stack->prev->reachable_labels, label->value))
+	  gfc_error ("GOTO statement at %L leaves CRITCAL block for label at "
+		     "%L", &code->loc, &label->where);
+
+      return;
+    }

I'm sure this is correct, but please enlighten me what reachable_labels 
does exactly store.  Is it here that a label inside the CRITICAL block 
is not reachable from the next outer block (ie. not allowed to jump 
inside a block) and thus branching to a label that *is* reachable from 
the parent must jump outside the block?  (Or something similar?)

Actually it seems to me that the comment "step three" above that block 
could be made more exact and you may want to add something there about 
your CRITICAL changes?

+    else if (p-> state == COMP_CRITICAL)

A " " too much.

+  if (st == ST_STOP || st == ST_ALL_STOP)
+    new_st.op = st == ST_STOP ? EXEC_STOP : EXEC_ALL_STOP;
+  else
+    new_st.op = EXEC_PAUSE;

What about a switch here instead of this somewhat "nested" if construction?

+      m =gfc_match_char ('*');

Here's a " " missing, in turn ;)

+  if (st == ST_SYNC_ALL)
+    new_st.op = EXEC_SYNC_ALL;
+  else if (st == ST_SYNC_IMAGES)
+    new_st.op = EXEC_SYNC_IMAGES;
+  else
+    new_st.op = EXEC_SYNC_MEMORY;

This is of course clearer than the if construction mentioned above, but 
I'd probably also use a switch here.

Thanks for the patch!  I hope we can work out some restricted (or even 
real) co-array support for 4.6!

Yours,
Daniel

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