Re: [PATCH] fb: sm501fb off-by-1 sysfs store

["Paul A. Clarke" <pc@us.ibm.com>]

Paul Mundt wrote:
> On Thu, Mar 01, 2007 at 02:25:55PM +0100, Geert Uytterhoeven wrote:
>> On Thu, 1 Mar 2007, Paul Mundt wrote:
>>> @@ -1074,9 +1074,9 @@ static ssize_t sm501fb_crtsrc_store(struct device *dev,
>>>  	if (len < 1)
>>>  		return -EINVAL;
>>>  
>>> -	if (strnicmp(buf, "crt", sizeof("crt")) == 0)
>>> +	if (strnicmp(buf, "crt", 3) == 0)
>>>  		head = HEAD_CRT;
>>> -	else if (strnicmp(buf, "panel", sizeof("panel")) == 0)
>>> +	else if (strnicmp(buf, "panel", 5) == 0)
>>>  		head = HEAD_PANEL;
>>>  	else
>>>  		return -EINVAL;
>> What about using just `strlen("string")' instead? ISTR gcc optimizes this to a
>> constant. Not that it matters that much, as the actual string won't be
>> duplicated.
>>
> I stuck with the constants since they were already referenced in the
> function, and there are enough other places in the kernel where they're
> used for these things. I didn't figure that 5 was a terribly high number
> to count to, but if there's expected difficulty, strlen() is of course an
> option.
> 
> Personally I find it less visually offensive with constants, but as long
> as the end number is the same, I'm not too concerned with how we get
> there. Feel free to change it if you wish.

My personal preference is to use sizeof("string")-1:
- guaranteed to be optimized to a constant (better than strlen)
- less likely to be left alone when "string" becomes "a different string" 
(better than magic numbers)

I also prefer for repeated constants, either preprocessor macros (#define 
OPTION_CRT "crt") or constant char arrays (static const char Option_CRT[] = 
"crt"), or something like that, to ensure one copy of the string in the object. 
  But having the indirection here could add confusion for somebody trying to 
quickly determine what "crt" does.
-- 
Regards,
Paul Clarke

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV