Messages in this thread | | | Date | Thu, 26 May 2011 07:43:05 -0600 | From | Tim Gardner <> | Subject | Re: [PATCH] drivers: video: Remove useless checks |
| |
On 05/26/2011 03:30 AM, Maxin B John wrote: > Hi, > > On Wed, May 25, 2011 at 1:47 PM, Tim Gardner<tim.gardner@canonical.com> wrote: >> On 05/19/2011 12:27 PM, Maxin B John wrote: >>> >>> Comparing unsigned less than zero will never be true. >>> Removing similar checks from 'fbmem.c' and 'fbcmap.c'. >> >> Looks right to me, though there are other places that suffer from the same >> issue. See fb_set_cmap() and its use of 'int start' and cmap->start. > > Thank you very much for reviewing the patch. As per your suggestion, I > have checked > the scenario is fb_set_cmap() and the use of 'int start' and 'cmap->start'. > > IMHO, that scenario doesn't fall under the comparison of unsigned int > < 0. That scenario > looks similar to the below given code to me: > /*---------------------------------*/ > #include<stdio.h> > > int > main () > { > unsigned int u_int = -1; > int s_int = 0; > s_int = u_int; > if (s_int< 0) > printf ("s_int is less than 0\n"); > return 0; > } > /*---------------------------------*/ > > Please let me know your comments. > > Best Regards, > Maxin >
IMHO mixing signed and unsigned comparisons like this is just wrong. Its unnecessarily complicated and misleading, especially for a device driver. I'm a firm believer in the KISS philosophy for driver development.
Its likely that the reason the fb code got into this situation is because a type was changed from signed to unsigned whence long ago, and nobody has bothered to clean it up.
rtg -- Tim Gardner tim.gardner@canonical.com
| |