Re: [Patch, Fortran] (Coarray) Add parse support for LOCK/UNLOCK (part 1 of 2)

[Tobias Burnus <burnus at net-b dot de>]

Dear Daniel,

thank you for the review.

Daniel Kraft wrote:
> +      fputs ("lock-variable=", dumpfile);
> +      if (c->expr1 != NULL)
> +	show_expr (c->expr1);
>
> Why do you dump "lock-variable=" in any case, while you only print the
> names for the other arguments only if present?

The lock variable is required and thus always present; the others 
(stat=, errmgs=, and lock_acquired=) are optional.

> - -  gfc_expr *expr1, *expr2, *expr3;
> +  gfc_expr *expr1, *expr2, *expr3, *expr4;
>
> Just a side-remark, but this makes me wonder whether we should at some
> point use a union there if we keep adding more and more expressions?  So
> that the code can be understood more easily and it is always clear what
> something like c->expr3 actually references?

Maybe. Though at least expr1 and expr2 are used by most statements thus 
it will be rather invasive.

> +          tmp = NULL;
> +	  break;
>
> Looks like a white-space / tab mismatch in the tmp = NULL line.

Fixed.

> For the same context (lock_unlock_statement function):  We're repeating
> the same matching logic thrice for all stat-variables ... maybe I would
> be tempted to think about a way out; possibly using a macro.  Although
> this may of course also make the code harder to read.  I'm certainly ok
> with the code as it is, just a thought.  (I personally don't really like
> duplicating code so large, although it is a very simple and clear one.)

I left it as is. I agree that it is a code duplication, but I think a 
macro does not really make it readable and three items - all which are 
relatively short - is small enough to tolerate the duplication.

Tobias