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

[Gaëtan Rivet]

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.

> 	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