[patch] staging: comedi: cleanup comedi_recognize()

[patch] staging: comedi: cleanup comedi_recognize()

[Dan Carpenter]

This function is more complicated than it needs to be because of the
consts.  It's not worth saving them because we drop the consts anyway
when we return (void *)name_ptr.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 1c3d638..a8f576d 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -304,14 +304,13 @@ static int postconfig(struct comedi_device *dev)
  * that register their supported board names */
 static void *comedi_recognize(struct comedi_driver *driv, const char *name)
 {
-	unsigned i;
-	const char *const *name_ptr = driv->board_name;
+	char **name_ptr = (char **)driv->board_name;
+	int i;
+
 	for (i = 0; i < driv->num_names; i++) {
 		if (strcmp(*name_ptr, name) = 0)
-			return (void *)name_ptr;
-		name_ptr -		    (const char *const *)((const char *)name_ptr +
-					  driv->offset);
+			return name_ptr;
+		name_ptr = (char **)((void *)name_ptr + driv->offset);
 	}
 
 	return NULL;

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Ian Abbott]

On 2012-05-24 11:28, Dan Carpenter wrote:
> This function is more complicated than it needs to be because of the
> consts.  It's not worth saving them because we drop the consts anyway
> when we return (void *)name_ptr.
>
> Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
>
> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index 1c3d638..a8f576d 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -304,14 +304,13 @@ static int postconfig(struct comedi_device *dev)
>    * that register their supported board names */
>   static void *comedi_recognize(struct comedi_driver *driv, const char *name)
>   {
> -	unsigned i;
> -	const char *const *name_ptr = driv->board_name;
> +	char **name_ptr = (char **)driv->board_name;
> +	int i;
> +
>   	for (i = 0; i<  driv->num_names; i++) {
>   		if (strcmp(*name_ptr, name) = 0)
> -			return (void *)name_ptr;
> -		name_ptr > -		    (const char *const *)((const char *)name_ptr +
> -					  driv->offset);
> +			return name_ptr;
> +		name_ptr = (char **)((void *)name_ptr + driv->offset);
>   	}
>
>   	return NULL;

You could simplify it further by keeping one of the consts:

static void *comedi_recognize(struct comedi_driver *driv, const char *name)
{
	unsigned i;
	const char **name_ptr = driv->board_name;
	for (i = 0; i < driv->num_names; i++) {
		if (strcmp(*name_ptr, name) = 0)
			return name_ptr;
		name_ptr = (void *)name_ptr + driv->offset;
	}
	return NULL;
}

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Dan Carpenter]

On Thu, May 24, 2012 at 11:57:44AM +0100, Ian Abbott wrote:
> On 2012-05-24 11:28, Dan Carpenter wrote:
> >This function is more complicated than it needs to be because of the
> >consts.  It's not worth saving them because we drop the consts anyway
> >when we return (void *)name_ptr.
> >
> >Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
> >
> >diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> >index 1c3d638..a8f576d 100644
> >--- a/drivers/staging/comedi/drivers.c
> >+++ b/drivers/staging/comedi/drivers.c
> >@@ -304,14 +304,13 @@ static int postconfig(struct comedi_device *dev)
> >   * that register their supported board names */
> >  static void *comedi_recognize(struct comedi_driver *driv, const char *name)
> >  {
> >-	unsigned i;
> >-	const char *const *name_ptr = driv->board_name;
> >+	char **name_ptr = (char **)driv->board_name;
> >+	int i;
> >+
> >  	for (i = 0; i<  driv->num_names; i++) {
> >  		if (strcmp(*name_ptr, name) = 0)
> >-			return (void *)name_ptr;
> >-		name_ptr > >-		    (const char *const *)((const char *)name_ptr +
> >-					  driv->offset);
> >+			return name_ptr;
> >+		name_ptr = (char **)((void *)name_ptr + driv->offset);
> >  	}
> >
> >  	return NULL;
> 
> You could simplify it further by keeping one of the consts:
> 
> static void *comedi_recognize(struct comedi_driver *driv, const char *name)
> {
> 	unsigned i;
> 	const char **name_ptr = driv->board_name;
> 	for (i = 0; i < driv->num_names; i++) {
> 		if (strcmp(*name_ptr, name) = 0)
> 			return name_ptr;
> 		name_ptr = (void *)name_ptr + driv->offset;
> 	}
> 	return NULL;
> }

Sure.  I'll resend.

regards,
dan carpenter

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Ian Abbott]

On 2012-05-24 12:16, Dan Carpenter wrote:
> On Thu, May 24, 2012 at 11:57:44AM +0100, Ian Abbott wrote:
>> On 2012-05-24 11:28, Dan Carpenter wrote:
>>> This function is more complicated than it needs to be because of the
>>> consts.  It's not worth saving them because we drop the consts anyway
>>> when we return (void *)name_ptr.
>>>
>>> Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
>>> index 1c3d638..a8f576d 100644
>>> --- a/drivers/staging/comedi/drivers.c
>>> +++ b/drivers/staging/comedi/drivers.c
>>> @@ -304,14 +304,13 @@ static int postconfig(struct comedi_device *dev)
>>>    * that register their supported board names */
>>>   static void *comedi_recognize(struct comedi_driver *driv, const char *name)
>>>   {
>>> -	unsigned i;
>>> -	const char *const *name_ptr = driv->board_name;
>>> +	char **name_ptr = (char **)driv->board_name;
>>> +	int i;
>>> +
>>>   	for (i = 0; i<   driv->num_names; i++) {
>>>   		if (strcmp(*name_ptr, name) = 0)
>>> -			return (void *)name_ptr;
>>> -		name_ptr >>> -		    (const char *const *)((const char *)name_ptr +
>>> -					  driv->offset);
>>> +			return name_ptr;
>>> +		name_ptr = (char **)((void *)name_ptr + driv->offset);
>>>   	}
>>>
>>>   	return NULL;
>>
>> You could simplify it further by keeping one of the consts:
>>
>> static void *comedi_recognize(struct comedi_driver *driv, const char *name)
>> {
>> 	unsigned i;
>> 	const char **name_ptr = driv->board_name;
>> 	for (i = 0; i<  driv->num_names; i++) {
>> 		if (strcmp(*name_ptr, name) = 0)
>> 			return name_ptr;
>> 		name_ptr = (void *)name_ptr + driv->offset;
>> 	}
>> 	return NULL;
>> }
>
> Sure.  I'll resend.

Actually, my version gives a warning about a dropped qualifier, so the 
initializer of name_ptr would need a cast after all.  You could keep 
your version but remove the casts from (void *) when converting to the 
other pointer types.

static void *comedi_recognize(struct comedi_driver *driv, const char *name)
{
	unsigned i;
	char **name_ptr = (char **)driv->board_name;
	for (i = 0; i < driv->num_names; i++) {
		if (strcmp(*name_ptr, name) = 0)
			return name_ptr;
		name_ptr = (void *)name_ptr + driv->offset;
	}

	return NULL;
}

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [patch] staging: comedi: cleanup comedi_recognize()

[walter harms]

Am 24.05.2012 12:57, schrieb Ian Abbott:
> On 2012-05-24 11:28, Dan Carpenter wrote:
>> This function is more complicated than it needs to be because of the
>> consts.  It's not worth saving them because we drop the consts anyway
>> when we return (void *)name_ptr.
>>
>> Signed-off-by: Dan Carpenter<dan.carpenter@oracle.com>
>>
>> diff --git a/drivers/staging/comedi/drivers.c
>> b/drivers/staging/comedi/drivers.c
>> index 1c3d638..a8f576d 100644
>> --- a/drivers/staging/comedi/drivers.c
>> +++ b/drivers/staging/comedi/drivers.c
>> @@ -304,14 +304,13 @@ static int postconfig(struct comedi_device *dev)
>>    * that register their supported board names */
>>   static void *comedi_recognize(struct comedi_driver *driv, const char
>> *name)
>>   {
>> -    unsigned i;
>> -    const char *const *name_ptr = driv->board_name;
>> +    char **name_ptr = (char **)driv->board_name;
>> +    int i;
>> +
>>       for (i = 0; i<  driv->num_names; i++) {
>>           if (strcmp(*name_ptr, name) = 0)
>> -            return (void *)name_ptr;
>> -        name_ptr >> -            (const char *const *)((const char *)name_ptr +
>> -                      driv->offset);
>> +            return name_ptr;
>> +        name_ptr = (char **)((void *)name_ptr + driv->offset);
>>       }
>>
>>       return NULL;
> 
> You could simplify it further by keeping one of the consts:
> 
> static void *comedi_recognize(struct comedi_driver *driv, const char *name)
> {
>     unsigned i;
>     const char **name_ptr = driv->board_name;
>     for (i = 0; i < driv->num_names; i++) {
>         if (strcmp(*name_ptr, name) = 0)
>             return name_ptr;
>         name_ptr = (void *)name_ptr + driv->offset;
>     }
>     return NULL;
> }
> 

Hi all,
the patch is fine with me but i have a few basic questions:

Why the (void  *) at all ? it returns a name what is a const char *.

A look at the structure also shows that this is a linked list, why
using offset instead of driv->next ?

re,
 wh

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Dan Carpenter]

On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
> Hi all,
> the patch is fine with me but i have a few basic questions:
> 
> Why the (void  *) at all ? it returns a name what is a const char *.
> 

We're really returning a pointer to a private struct, it's just that
the first element on the struct always has to be a pointer to char *.

> A look at the structure also shows that this is a linked list, why
> using offset instead of driv->next ?
> 

It's a different list.

regards,
dan carpenter

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Ian Abbott]

On 2012-06-05 12:01, Dan Carpenter wrote:
> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
>> Hi all,
>> the patch is fine with me but i have a few basic questions:
>>
>> Why the (void  *) at all ? it returns a name what is a const char *.
>>
>
> We're really returning a pointer to a private struct, it's just that
> the first element on the struct always has to be a pointer to char *.

To be pedantic, it's really returning a pointer to some member of type 
'const char *' within a private struct.  To make life easier for 
themselves, those drivers make that the first member of the private 
struct so a void pointer to the member is also a void pointer to the struct.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [patch] staging: comedi: cleanup comedi_recognize()

[walter harms]

Am 06.06.2012 11:28, schrieb Ian Abbott:
> On 2012-06-05 12:01, Dan Carpenter wrote:
>> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
>>> Hi all,
>>> the patch is fine with me but i have a few basic questions:
>>>
>>> Why the (void  *) at all ? it returns a name what is a const char *.
>>>
>>
>> We're really returning a pointer to a private struct, it's just that
>> the first element on the struct always has to be a pointer to char *.
> 
> To be pedantic, it's really returning a pointer to some member of type
> 'const char *' within a private struct.  To make life easier for
> themselves, those drivers make that the first member of the private
> struct so a void pointer to the member is also a void pointer to the
> struct.
> 

I do not like it. It is confusing.
Are these struct comedi_driver *driv of different size every time ?
or why is it not possible to return simply driv[i] ?

re,
 wh

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Dan Carpenter]

On Wed, Jun 06, 2012 at 11:45:43AM +0200, walter harms wrote:
> 
> 
> Am 06.06.2012 11:28, schrieb Ian Abbott:
> > On 2012-06-05 12:01, Dan Carpenter wrote:
> >> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
> >>> Hi all,
> >>> the patch is fine with me but i have a few basic questions:
> >>>
> >>> Why the (void  *) at all ? it returns a name what is a const char *.
> >>>
> >>
> >> We're really returning a pointer to a private struct, it's just that
> >> the first element on the struct always has to be a pointer to char *.
> > 
> > To be pedantic, it's really returning a pointer to some member of type
> > 'const char *' within a private struct.  To make life easier for
> > themselves, those drivers make that the first member of the private
> > struct so a void pointer to the member is also a void pointer to the
> > struct.
> > 
> 
> I do not like it. It is confusing.

Yep.

> Are these struct comedi_driver *driv of different size every time ?

Nope.

regards,
dan carpenter

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Ian Abbott]

On 2012-06-06 10:49, Dan Carpenter wrote:
> On Wed, Jun 06, 2012 at 11:45:43AM +0200, walter harms wrote:
>>
>>
>> Am 06.06.2012 11:28, schrieb Ian Abbott:
>>> On 2012-06-05 12:01, Dan Carpenter wrote:
>>>> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
>>>>> Hi all,
>>>>> the patch is fine with me but i have a few basic questions:
>>>>>
>>>>> Why the (void  *) at all ? it returns a name what is a const char *.
>>>>>
>>>>
>>>> We're really returning a pointer to a private struct, it's just that
>>>> the first element on the struct always has to be a pointer to char *.
>>>
>>> To be pedantic, it's really returning a pointer to some member of type
>>> 'const char *' within a private struct.  To make life easier for
>>> themselves, those drivers make that the first member of the private
>>> struct so a void pointer to the member is also a void pointer to the
>>> struct.
>>>
>>
>> I do not like it. It is confusing.
>
> Yep.

But at least the confusion is concentrated in a single place!  From the 
individual drivers' point of view it's fairly simple (as long as it puts 
the 'const char pointer to board name' member at the top of its private 
data structure).

>> Are these struct comedi_driver *driv of different size every time ?
>
> Nope.

It's the private board information data structures for each driver that 
are different sizes, but the size is passed in the comedi comedi_driver 
'offset' member along with the 'board_name' and 'num_names' members. 
Not all drivers set those members and just use the 'driver_name' to 
match any supported device, but I digress.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

Re: [patch] staging: comedi: cleanup comedi_recognize()

[walter harms]

Am 06.06.2012 13:20, schrieb Ian Abbott:
> On 2012-06-06 10:49, Dan Carpenter wrote:
>> On Wed, Jun 06, 2012 at 11:45:43AM +0200, walter harms wrote:
>>>
>>>
>>> Am 06.06.2012 11:28, schrieb Ian Abbott:
>>>> On 2012-06-05 12:01, Dan Carpenter wrote:
>>>>> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
>>>>>> Hi all,
>>>>>> the patch is fine with me but i have a few basic questions:
>>>>>>
>>>>>> Why the (void  *) at all ? it returns a name what is a const char *.
>>>>>>
>>>>>
>>>>> We're really returning a pointer to a private struct, it's just that
>>>>> the first element on the struct always has to be a pointer to char *.
>>>>
>>>> To be pedantic, it's really returning a pointer to some member of type
>>>> 'const char *' within a private struct.  To make life easier for
>>>> themselves, those drivers make that the first member of the private
>>>> struct so a void pointer to the member is also a void pointer to the
>>>> struct.
>>>>
>>>
>>> I do not like it. It is confusing.
>>
>> Yep.
> 
> But at least the confusion is concentrated in a single place!  From the
> individual drivers' point of view it's fairly simple (as long as it puts
> the 'const char pointer to board name' member at the top of its private
> data structure).
> 

Sometimes you can not write "clean" code but a few lines of comments are
then a must.


>>> Are these struct comedi_driver *driv of different size every time ?
>>
>> Nope.
> 
> It's the private board information data structures for each driver that
> are different sizes, but the size is passed in the comedi comedi_driver
> 'offset' member along with the 'board_name' and 'num_names' members. Not
> all drivers set those members and just use the 'driver_name' to match
> any supported device, but I digress.
> 
If i can believe the comments in the structure offset is the distance to the next
name  /* offset in bytes from one board name pointer to the next */
and the name is not the first entry it is the second.
That means any program assuming that this code returns a (struct comedi_driver *)
has to do some correction or it will be off-bye-one sizeof(struct comedi_driver *).
Please correct me if i am wrong:

I have no clue how dev->board_ptr is used, if it works it is fine with me.

re,
 wh

Re: [patch] staging: comedi: cleanup comedi_recognize()

[Ian Abbott]

On 2012-06-06 13:28, walter harms wrote:
> Am 06.06.2012 13:20, schrieb Ian Abbott:
>> On 2012-06-06 10:49, Dan Carpenter wrote:
>>> On Wed, Jun 06, 2012 at 11:45:43AM +0200, walter harms wrote:
>>>>
>>>>
>>>> Am 06.06.2012 11:28, schrieb Ian Abbott:
>>>>> On 2012-06-05 12:01, Dan Carpenter wrote:
>>>>>> On Tue, Jun 05, 2012 at 12:06:10PM +0200, walter harms wrote:
>>>>>>> Hi all,
>>>>>>> the patch is fine with me but i have a few basic questions:
>>>>>>>
>>>>>>> Why the (void  *) at all ? it returns a name what is a const char *.
>>>>>>>
>>>>>>
>>>>>> We're really returning a pointer to a private struct, it's just that
>>>>>> the first element on the struct always has to be a pointer to char *.
>>>>>
>>>>> To be pedantic, it's really returning a pointer to some member of type
>>>>> 'const char *' within a private struct.  To make life easier for
>>>>> themselves, those drivers make that the first member of the private
>>>>> struct so a void pointer to the member is also a void pointer to the
>>>>> struct.
>>>>>
>>>>
>>>> I do not like it. It is confusing.
>>>
>>> Yep.
>>
>> But at least the confusion is concentrated in a single place!  From the
>> individual drivers' point of view it's fairly simple (as long as it puts
>> the 'const char pointer to board name' member at the top of its private
>> data structure).
>>
>
> Sometimes you can not write "clean" code but a few lines of comments are
> then a must.
>
>
>>>> Are these struct comedi_driver *driv of different size every time ?
>>>
>>> Nope.
>>
>> It's the private board information data structures for each driver that
>> are different sizes, but the size is passed in the comedi comedi_driver
>> 'offset' member along with the 'board_name' and 'num_names' members. Not
>> all drivers set those members and just use the 'driver_name' to match
>> any supported device, but I digress.
>>
> If i can believe the comments in the structure offset is the distance to the next
> name  /* offset in bytes from one board name pointer to the next */
> and the name is not the first entry it is the second.

This board name pointer is in a driver-specific, board information 
struct.  All of them make it the first member of the driver-specific 
struct for convenience, although they don't need to - they could use 
offsetof() or container_of() to convert the pointer to the member to a 
pointer to the containing struct.  The comedi core does not care about 
the driver-specific struct but needs to be told how to access the board 
names in an initialized array of these structs.

> That means any program assuming that this code returns a (struct comedi_driver *)
> has to do some correction or it will be off-bye-one sizeof(struct comedi_driver *).
> Please correct me if i am wrong:
>
> I have no clue how dev->board_ptr is used, if it works it is fine with me.

If the low-level driver initializes 'board_name', 'offset' and 
'num_names' to something non-zero in its 'struct comedi_driver' then 
'dev->board_ptr' will point to one of the board name pointers in 
initialized array of driver-specific, board information structs when the 
driver's attach() hook is called.  If 'num_names' is 0, 'dev->board_ptr' 
will be NULL when the attach() hook is called.  In either case, the 
low-level driver is free to change the value of 'dev->board_ptr' for its 
own purposes, although generally it will be pointed to some element of 
its array of board information structs.

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-