From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.warmcat.com (mail.warmcat.com [163.172.24.82]) by dpdk.org (Postfix) with ESMTP id AA5A52B9D for ; Tue, 22 May 2018 03:25:29 +0200 (CEST) To: Bruce Richardson Cc: dev@dpdk.org, thomas@monjalon.net References: <152686781484.58694.14737673447518527445.stgit@localhost.localdomain> <152686809351.58694.1376497095015990878.stgit@localhost.localdomain> <20180521133022.GH22944@bricha3-MOBL.ger.corp.intel.com> From: Andy Green Message-ID: <3ce8da50-2814-35f4-b4df-e4d4ae7eef29@warmcat.com> Date: Tue, 22 May 2018 09:25:23 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 In-Reply-To: <20180521133022.GH22944@bricha3-MOBL.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 8/8] 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: Tue, 22 May 2018 01:25:29 -0000 On 05/21/2018 09:30 PM, Bruce Richardson wrote: > On Mon, May 21, 2018 at 10:01:33AM +0800, Andy Green wrote: >> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h: >> In function 'rte_eth_rx_burst': >> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h: >> 3836:18: warning: conversion to 'int16_t' {aka 'short >> int'} from 'uint16_t' {aka 'short unsigned int'} may >> change the sign of the result [-Wsign-conversion] >> int16_t nb_rx = (*dev->rx_pkt_burst)(dev->data-> >> rx_queues[queue_id], >> ^ >> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h: >> 3844:50: warning: conversion to 'uint16_t' {aka 'short >> unsigned int'} from 'int16_t' {aka 'short int'} may >> change the sign of the result [-Wsign-conversion] >> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx, >> ^~~~~ >> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h: >> 3844:12: warning: conversion to 'int16_t' {aka 'short >> int'} from 'uint16_t' {aka 'short unsigned int'} may >> change the sign of the result [-Wsign-conversion] >> nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx, >> ^~ >> /projects/lagopus/src/dpdk/build/include/rte_ethdev.h: >> 3851:9: warning: conversion to 'uint16_t' {aka 'short >> unsigned int'} from 'int16_t' {aka 'short int'} may >> change the sign of the result [-Wsign-conversion] >> return nb_rx; >> ^~~~~ >> >> The second part of the patch is solved by its own basic >> block because it is inside a preprocessor conditional. >> >> Bringing the declaration of the var to the top of the >> function would require that also being given its own >> preprocessor conditional, or a (void)var to avoid an >> unused var warning. The basic block is no worse than >> those imho. >> >> Signed-off-by: Andy Green >> --- >> lib/librte_ethdev/rte_ethdev.h | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h >> index d52c1bed9..4d059a2a7 100644 >> --- a/lib/librte_ethdev/rte_ethdev.h >> +++ b/lib/librte_ethdev/rte_ethdev.h >> @@ -3823,6 +3823,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); >> @@ -3833,18 +3834,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); >> + } >> } > > I think you can eliminate these extra brackets by moving the cb declaration > inside the if body: > > if (unlikely(dev->post_rx_burst_cbs[queue_id] != NULL)) {$ > struct rte_eth_rxtx_callback *cb = > dev->post_rx_burst_cbs[queue_id]; > do {$ > nb_rx = cb->fn.rx(port_id, queue_id, rx_pkts, nb_rx,$ > nb_pkts, cb->param);$ > cb = cb->next;$ > } while (cb != NULL);$ > } Okay... it repeats the expression but what the hey. It's changed to that in the v7 just pushed. -Andy > /Bruce >