Re: [PATCH] allow user-configurable menucolor

["Vesa Jääskeläinen" <chaac@nic.fi>]

Robert Millan wrote:
> On Tue, Jan 01, 2008 at 03:14:50PM +0200, Vesa Jääskeläinen wrote:
>> First some questions ;)
>>
>> - What happens when you forgot to provide / in this string ;) ?
> 
> grub_strchr() returns NULL and... whoops!  I'll fix that :-)
> 
>> - What happens when all memory is used?
> 
> What do you mean?

strdup() == NULL

>> - What happens when you alter list of colors... remove one entry of it
>> or add one.
> 
> This should never happen.  The list is just a human description of GRUB color
> codes.  GRUB VGA-style color codes are never changed in runtime, so neither
> should this list.

Yes, but... (below)

>> (eg. hard coding is bad... sizeof...)
> 
> Where can sizeof be used?

for (i = 0; i < 0x10; i++)
->
for (i = 0; i < sizeof(color_list) / sizeof(*color_list); i++)

or similar compile time calculation for it. If you are using construct
like this then your code always indexes array correctly. This of course
requires that color_list is local to source file being compiled. Point
being taken here is that never use hard-coded indexes to some arrays
when there is even slight possibility that it can change.

>> Now to your naming issue...
>>
>> This code is quite specific to be only local so I would propose following:
>>
>> parse_single_color_name -> parse_color_name
>>
>> parse_color_name -> parse_color_tuple, parse_color_pair,
>> parse_text_color or parse_menu_color
> 
> How about `parse_color_name_pair' ?

this name is good as any and goes well with parse_color_name name.