From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?iso-8859-1?Q?Ga=EBtan?= Rivet <gaetan.rivet@6wind.com>
Subject: Re: [PATCH v6 09/11] pci: implement find_device bus
 operation
Date: Wed, 28 Jun 2017 11:17:46 +0200
Message-ID: <20170628091746.GG13355@bidouze.vm.6wind.com>
References: <cover.1498436062.git.gaetan.rivet@6wind.com>
 <cover.1498577192.git.gaetan.rivet@6wind.com>
 <5fe2e5fb2759d1f6d3f86e5dfae5c3fc177299da.1498577192.git.gaetan.rivet@6wind.com>
 <20170627163514.2ed2ec6b@xeon-e3>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Cc: dev@dpdk.org
To: Stephen Hemminger <stephen@networkplumber.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54])
 by dpdk.org (Postfix) with ESMTP id 9EAC52BE1
 for <dev@dpdk.org>; Wed, 28 Jun 2017 11:17:55 +0200 (CEST)
Received: by mail-wm0-f54.google.com with SMTP id 62so47203126wmw.1
 for <dev@dpdk.org>; Wed, 28 Jun 2017 02:17:55 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20170627163514.2ed2ec6b@xeon-e3>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

On Tue, Jun 27, 2017 at 04:35:14PM -0700, Stephen Hemminger wrote:
> On Tue, 27 Jun 2017 18:11:16 +0200
> Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> 
> > +	int start_found = !!(start == NULL);
> > +
> > +	FOREACH_DEVICE_ON_PCIBUS(dev) {
> > +		if (!start_found) {
> > +			if (&dev->device == start)
> > +				start_found = 1;
> > +			continue;
> > +		}
> > +		if (cmp(&dev->device, data) == 0)
> > +			return &dev->device;
> > +	}
> > +	return NULL;
> > +}
> > +
> 
> Why is start_found not a boolean?
> 

Ah, yes, I wrote this a few times over in rte_bus and rte_vdev, and
mostly used the same scheme in the PCI implementation, without checking
for the use of stdbool in the vincinity otherwise.

I would not be opposed to using a bool if I was rewriting it, but I
don't feel this change warrants a new version.

> Do you really need start_found at all? Why not just reuse existing
> pointer?
> 

You are right, it could be reduced. But I find it a little too "clever"
in a sense, and I prefer usually to avoid rewriting input parameters. If this
function had to be refactored later, the writer would need to be careful
about start having changed. The advantage of using one variable less
does not outweight this drawback I think.

> 	FOREACH_DEVICE_ON_PCIBUS(dev) {
> 		if (start) {
> 			if (&dev->device == start)
> 				start = NULL
> 			continue;
> 		}
> 		if (cmp(&dev->device, data) == 0)
> 			return &dev->device;
> 	}
> 	return NULL;
> }

-- 
Gaëtan Rivet
6WIND