[PATCH 1/2] ui/ncurses: Add function to insert boot entries in specific position
Jeremy Kerr
jk at ozlabs.org
Thu Jul 3 16:52:27 EST 2014
Hi Sam,
> pmenu_item_add() is added in preparation for changes to the ncurses
> UI where boot entries are grouped underneath the name of the matching
> boot device
I'd be fine with merging this with the previous patch.
Just a couple of minimal things:
> +void pmenu_item_add(struct pmenu *menu, struct pmenu_item *item, unsigned int insert_pt)
Could trim this to 80 chars? Might also be good to mention that the menu
must have preallocated space for the new item.
> + int found = 0;
May as well make this a bool
> + struct cui_opt_data *cod = item->data;;
Duplicate semicolon here.
> + if(menu->items[dev]){
Just needs a couple of spaces around the brackets:
if (menu->items[dev]) {
- same applies to other places in your two patches.
> + if(found){
> + assert(dev<insert_pt);
Really minor, but spaces around the operator here make it a little more
readable.
> + /* Shift down entries between header and insert_pt */
> + memmove(menu->items + dev + 2,
> + menu->items + dev + 1,
> + ((menu->items + insert_pt) - (menu->items + dev + 1)) * sizeof(ITEM *));
I'd have a slight preference for sizeof(menu->items[0]) here, so we
don't have problems if the type of the items array changes. Also, 80
chars.
> + memset(menu->items + dev + 1, 0, sizeof(ITEM *));
> + pmenu_item_insert(menu, item, dev + 1);
> + } else {
> + /* For some reason we didn't find the matching device;
> + * at least add it to a valid position */
> + pmenu_item_insert(menu, item, insert_pt);
> + }
> +}
Since we need to do a pmenu_item_insert in both cases, you could just do
a:
if (found) {
[...]
insert_pt = dev + 1;
}
pmenu_item_insert(menu, item, insert_pt);
Cheers,
Jeremy
More information about the Petitboot
mailing list