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
