[dpdk-dev] [PATCH v6 09/11] pci: implement find_device bus operation

[Richardson, Bruce]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Gaëtan Rivet
> Sent: Wednesday, June 28, 2017 10:18 AM
> To: Stephen Hemminger <stephen at networkplumber.org>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v6 09/11] pci: implement find_device bus
> operation
> 
> 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 at 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.
> 
+1 for having an extra variable for clarity.