From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 91BC85F72 for ; Thu, 17 May 2018 16:26:22 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 May 2018 07:26:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,410,1520924400"; d="scan'208";a="59455863" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.55]) by orsmga002.jf.intel.com with SMTP; 17 May 2018 07:26:19 -0700 Received: by (sSMTP sendmail emulation); Thu, 17 May 2018 15:26:18 +0100 Date: Thu, 17 May 2018 15:26:17 +0100 From: Bruce Richardson To: Andy Green Cc: dev@dpdk.org Message-ID: <20180517142617.GE22288@bricha3-MOBL.ger.corp.intel.com> References: <152627436523.53156.4398253089110011263.stgit@localhost.localdomain> <152627464301.53156.5866882653553223698.stgit@localhost.localdomain> <20180517135417.GB22288@bricha3-MOBL.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v4 15/23] rte_ethdev.h: align sign and scope of temp var X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 May 2018 14:26:23 -0000 On Thu, May 17, 2018 at 10:17:04PM +0800, Andy Green wrote: > > > On 05/17/2018 09:54 PM, Bruce Richardson wrote: > > On Mon, May 14, 2018 at 01:10:43PM +0800, Andy Green wrote: > > > Signed-off-by: Andy Green > > > --- > > > lib/librte_ethdev/rte_ethdev.h | 25 +++++++++++++++---------- > > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > > > > While I dislike the changes below, since I believe it's always more > > readable to declare variables at first use, if the changes are needed to > > remove compiler errors in apps, then they need to be fixed. > > > > Patch needs a suitable commit log explaining the changes or giving the > > error message. > > It has this in the last push (which overlapped with your comment)... I seem > to have missed the error about declarations after code not being allowed in > C90, but I guess that part being an issue is not controversial. > > > > > > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h > > > index 49c2ebbd5..2cb5fe3be 100644 > > > --- a/lib/librte_ethdev/rte_ethdev.h > > > +++ b/lib/librte_ethdev/rte_ethdev.h > > > @@ -3801,6 +3801,7 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id, > > > struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) > > > { > > > struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > > + uint16_t nb_rx; > > > #ifdef RTE_LIBRTE_ETHDEV_DEBUG > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0); > > > @@ -3811,18 +3812,22 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id, > > > return 0; > > > } > > > #endif > > > - int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], > > > - rx_pkts, nb_pkts); > > > + nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], > > > + rx_pkts, nb_pkts); > > > #ifdef RTE_ETHDEV_RXTX_CALLBACKS > > > - struct rte_eth_rxtx_callback *cb = dev->post_rx_burst_cbs[queue_id]; > > > - > > > - if (unlikely(cb != NULL)) { > > > - do { > > > - nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx, > > > - nb_pkts, cb->param); > > > - cb = cb->next; > > > - } while (cb != NULL); > > > + { > > > + struct rte_eth_rxtx_callback *cb = > > > + dev->post_rx_burst_cbs[queue_id]; > > > + > > > + if (unlikely(cb != NULL)) { > > > + do { > > > + nb_rx = cb->fn.rx(port_id, queue_id, > > > + rx_pkts, nb_rx, > > > + nb_pkts, cb->param); > > > + cb = cb->next; > > > + } while (cb != NULL); > > > + } > > > } > > > #endif > > > > Rather than increasing the level of indentation needed with the extra > > braces, it's probably best to separate variable definition and assignment > > as you did in the first change above. > > IIRC my thinking was I had a choice to repeat the conditional compilation > stuff around the declaration (because it's in an #ifdef), (void)cb; to dodge > the unused var warning, or add a basic block for it to scope to. The last > one didn't seem so bad. > Ok, that makes sense. For completeness, I'd add a "do" and "while(0)" to the braces, but that's not important here, it's ok as-is. BTW: for your v5, I don't think you kept the previous ack's that myself and Olivier had given for a number of your patches. It's strongly recommended that you put any previously-given acks in any new revisions after the signoff line, so as to make it easier to track for maintainers, and to save us having to re-ack the same patches multiple times. Thanks, /Bruce