Re: [patch 1/2] ALSA: oxfw: some signedness bugs

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Dec 14 2014 02:30, Takashi Iwai wrote:
> At Sat, 13 Dec 2014 19:14:46 +0900,
> Takashi Sakamoto wrote:
>>
>> On Dec 13 2014 16:04, Dan Carpenter wrote:
>>> On Sat, Dec 13, 2014 at 02:06:59PM +0900, Takashi Sakamoto wrote:
>>>> On Dec 13 2014 04:27, Dan Carpenter wrote:
>>>>> This code tends to use unsigned variables by default and it causes
>>>>> signedness bugs when we use negative variables for error handling.
>>>>> The "i" and "j" variables are used to iterated over small positive
>>>>> values and so they should be type "int".  The "len" variable doesn't
>>>>> *need* to be signed but it should be signed to make the code easier to
>>>>> read and audit.
>>>>>
>>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Note that Dan's suggestions are about the variables like "i" and "j".
> These are used normally for small loop counts, and they are int in a
> standard idiom.  If they are declared as another type, you force
> reader's attention *unnecessarily*, and it decreases the readability
> (i.e. they have to read the loop code as somewhat special even if
> it's a normal loop).  This is the only big drawback, and the rest
> merit/demerit are almost ignorable, IMO.
> 
> Of course, in some cases, a loop count might be better in unsigned.
> But then a different variable name should be used instead.
> 
> After all, this is rather a minor issue, almost a bikeshed topic, so I
> didn't care much while reviewing your patches, and I still don't care
> whether this fix patch will have int or unsigned for i.  But, it'd be
> good if you keep this information in your mind, at least.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>


Thanks

Takashi Sakamoto
o-takashi@sakamocchi.jp