On Fri, Sep 18, 2020 at 09:40:43AM +0300, Sakari Ailus wrote:
> Hi Dan,
>
> On Thu, Sep 17, 2020 at 01:49:41PM +0300, Dan Carpenter wrote:
> > On Thu, Sep 17, 2020 at 01:33:43PM +0300, Sakari Ailus wrote:
> > > > +static int connect_supported_devices(void)
> > > > +{
> > > > + struct acpi_device *adev;
> > > > + struct device *dev;
> > > > + struct sensor_bios_data ssdb;
> > > > + struct sensor *sensor;
> > > > + struct property_entry *sensor_props;
> > > > + struct property_entry *cio2_props;
> > > > + struct fwnode_handle *fwnode;
> > > > + struct software_node *nodes;
> > > > + struct v4l2_subdev *sd;
> > > > + int i, ret;
> > >
> > > unsigned int i
> > >
> >
> > Why?
> >
> > For list iterators then "int i;" is best... For sizes then unsigned is
> > sometimes best. Or if it's part of the hardware spec or network spec
> > unsigned is best. Otherwise unsigned variables cause a ton of bugs.
> > They're not as intuitive as signed variables. Imagine if there is an
> > error in this loop and you want to unwind. With a signed variable you
> > can do:
> >
> > while (--i >= 0)
> > cleanup(&bridge.sensors[i]);
> >
> > There are very few times where raising the type maximum from 2 billion
> > to 4 billion fixes anything.
>
> There's simply no need for the negative integers here. Sizes (as it's a
> size here) are unsigned, too, so you'd be comparing signed and unsigned
> numbers later in the function.
I'm not trying to be rude, I'm honestly puzzled by this...
The "i" variable is not a size, it's an iterator... Comparing signed
and unsigned isn't necessarily a problem, but the only comparison in
this case is here:
253 struct property_entry *cio2_props;
254 struct fwnode_handle *fwnode;
255 struct software_node *nodes;
256 struct v4l2_subdev *sd;
257 int i, ret;
258
259 for (i = 0; i < ARRAY_SIZE(supported_devices); i++) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That's obviously fine. The compiler knows at compile time the value of
ARRAY_SIZE(). I feel like there must be a static checker which
complains about this? ARRAY_SIZE() is size_t.
260 adev = acpi_dev_get_first_match_dev(supported_devices[i],
261 NULL, -1);
262
263 if (!adev)
264 continue;
265
regards,
dan carpenter
