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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel