From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23476 invoked by alias); 8 Jun 2011 06:29:41 -0000 Received: (qmail 23339 invoked by uid 22791); 8 Jun 2011 06:29:40 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE X-Spam-Check-By: sourceware.org Received: from mx02.qsc.de (HELO mx02.qsc.de) (213.148.130.14) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Jun 2011 06:29:23 +0000 Received: from [192.168.178.22] (port-92-204-18-93.dynamic.qsc.de [92.204.18.93]) by mx02.qsc.de (Postfix) with ESMTP id CFEFE1E23B; Wed, 8 Jun 2011 08:29:19 +0200 (CEST) Message-ID: <4DEF16BE.1090409@net-b.de> Date: Wed, 08 Jun 2011 07:00:00 -0000 From: Tobias Burnus User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.2.17) Gecko/20110414 SUSE/3.1.10 Thunderbird/3.1.10 MIME-Version: 1.0 To: Daniel Kraft CC: gfortran , gcc patches Subject: Re: [Patch, Fortran] (Coarray) Add parse support for LOCK/UNLOCK (part 1 of 2) References: <4DED493E.5030705@net-b.de> <4DEDEB13.6030302@domob.eu> In-Reply-To: <4DEDEB13.6030302@domob.eu> Content-Type: text/plain; charset=ISO-8859-15; format=flowed Content-Transfer-Encoding: 7bit Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-06/txt/msg00611.txt.bz2 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