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