| Submitter | Carl-Daniel Hailfinger |
|---|---|
| Date | 2011-08-15 20:40:58 |
| Message ID | <4E49845A.8090202@gmx.net> |
| Download | mbox | patch |
| Permalink | /patch/3355/ |
| State | Accepted |
| Commit | r1424 |
| Headers | show |
Comments
On Mon, 15 Aug 2011 22:40:58 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > Am 03.08.2011 13:40 schrieb Stefan Tauner: > > On Wed, 03 Aug 2011 09:02:52 +0200 > > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > > > > >> Suggestion: > >> struct board_pciid_enable -> struct board_match > >> board_match_coreboot_name() -> board_match_cbname() > >> > >> What do you think? > >> > > looks fine imo. > > board_match_pci_card_ids could also be renamed to board_match_pci_ids > > and if you are at it please change the comment of it: > > > > Done. oh my. now that i see the code, i think renaming the board enables to board_match is really a bad idea. mainly because we use "match" as verb in other places, but here as substantive. e.g. static const struct board_match *board_match_cbname a function that matches boards according to their cbname should return a board, not a "board match". it is just a question of taste, but i find it awful :) the wiki table is named board_info hmhm maybe board_detail, but that's long.. :/ what about just "board"? another way to mitigate "my" problem would be to no use match as a verb for the method names, but using "get" or "find" instead. > > /* > > * Match boards on PCI IDs and subsystem IDs. > > * Second set of IDs can be main only or missing completely. > > */ > > const static struct board_pciid_enable *board_match_pci_card_ids(enum board_match_phase phase) > > > > - * Second set of IDs can be main only or missing completely. > > + * Second set of IDs can contain primary IDs only or be missing completely. > > > > I tried to find an alternate wording. that's good, because i obviously missed the point of the whole sentence/method ;) > > aaaand if we change board_match_pci_card_ids we should also change > > pci_card_find to pci_dev_find or something like that... :) > > > > Well, if we rename that one, we'd have to call it pci_dev_subsys_find, > and that's a net loss from the 80 column perspective, but it may indeed > clarify the code. Further input is appreciated. why do we have to? we find pci devs, not pci dev subsystems ;) hm maybe it should be find_pci_dev? or maybe no... still dizzy from sleeping sorry :)
Am 15.08.2011 23:42 schrieb Stefan Tauner: > On Mon, 15 Aug 2011 22:40:58 +0200 > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > >> Am 03.08.2011 13:40 schrieb Stefan Tauner: >> >>> On Wed, 03 Aug 2011 09:02:52 +0200 >>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: >>> >>> >>> >>>> Suggestion: >>>> struct board_pciid_enable -> struct board_match >>>> board_match_coreboot_name() -> board_match_cbname() >>>> >>>> What do you think? >>>> >>>> >>> looks fine imo. >>> board_match_pci_card_ids could also be renamed to board_match_pci_ids >>> and if you are at it please change the comment of it: >>> >>> >> Done. >> > oh my. now that i see the code, i think renaming the board enables to > board_match is really a bad idea. mainly because we use "match" as verb > in other places, but here as substantive. > e.g. > static const struct board_match *board_match_cbname > a function that matches boards according to their cbname should return > a board, not a "board match". it is just a question of taste, but i > find it awful :) > It's a trap! The struct is there to match a board, so struct board_match is exactly the right name for the _type_. The name of the function is another thing, you could probably call it match_board_match_cbname, but that would be silly, so we can either call it match_board_cbname or board_match_cbname. And the name of the variable which stores the result of the function call is even another thing, and there the name board might be appropriate. > the wiki table is named board_info hmhm maybe board_detail, but that's > long.. :/ > what about just "board"? > another way to mitigate "my" problem would be to no use match as a verb > for the method names, but using "get" or "find" instead. > "get" has the wrong semantic implication for the cbname matching, and "find" as in board_find_cbname suffers from the same problem because we're NOT interested in finding the cbname of the board, but rather in looking for a board which matches the supplied cbname. >>> aaaand if we change board_match_pci_card_ids we should also change >>> pci_card_find to pci_dev_find or something like that... :) >>> >>> >> Well, if we rename that one, we'd have to call it pci_dev_subsys_find, >> and that's a net loss from the 80 column perspective, but it may indeed >> clarify the code. Further input is appreciated. >> > why do we have to? we find pci devs, not pci dev subsystems ;) > hm maybe it should be find_pci_dev? > pci_dev_find() looks for a device matching the supplied vendor+dev IDs. pci_card_find() looks for a device matching the supplied vendor+dev+subvendor+subdev IDs. The name "card" was picked to allow differentiating between PCI devices (cards) which share vendor+dev ID, but have different subvendor+subdev IDs. That's pretty common for network cards where dozens of vendors use the same network chip (and thus fixed vendor+dev ID), but completely different PCBs. Regards, Carl-Daniel
On Tue, 16 Aug 2011 13:38:05 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > Am 15.08.2011 23:42 schrieb Stefan Tauner: > > On Mon, 15 Aug 2011 22:40:58 +0200 > > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > > > > > >> Am 03.08.2011 13:40 schrieb Stefan Tauner: > >> > >>> On Wed, 03 Aug 2011 09:02:52 +0200 > >>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > >>> > >>> > >>> > >>>> Suggestion: > >>>> struct board_pciid_enable -> struct board_match > >>>> board_match_coreboot_name() -> board_match_cbname() > >>>> > >>>> What do you think? > >>>> > >>>> > >>> looks fine imo. > >>> board_match_pci_card_ids could also be renamed to board_match_pci_ids > >>> and if you are at it please change the comment of it: > >>> > >>> > >> Done. > >> > > oh my. now that i see the code, i think renaming the board enables to > > board_match is really a bad idea. mainly because we use "match" as verb > > in other places, but here as substantive. > > e.g. > > static const struct board_match *board_match_cbname > > a function that matches boards according to their cbname should return > > a board, not a "board match". it is just a question of taste, but i > > find it awful :) > > > > It's a trap! > The struct is there to match a board, so struct board_match is exactly > the right name for the _type_. if that would be its only purpose, it would contain identification attributes only. last time i looked it had general info about boards (vendor and board name) and board enable specific fields (phase, max_rom etc). its previous name also suggests it is not just for matching. ;) > The name of the function is another > thing, you could probably call it match_board_match_cbname, but that > would be silly, so we can either call it match_board_cbname or > board_match_cbname. And the name of the variable which stores the result > of the function call is even another thing, and there the name board > might be appropriate. > > > > the wiki table is named board_info hmhm maybe board_detail, but that's > > long.. :/ > > what about just "board"? > > another way to mitigate "my" problem would be to no use match as a verb > > for the method names, but using "get" or "find" instead. > > > > "get" has the wrong semantic implication for the cbname matching, and > "find" as in board_find_cbname suffers from the same problem because > we're NOT interested in finding the cbname of the board, but rather in > looking for a board which matches the supplied cbname. i think the reason for our difference of opinion is how we view the table. for me it is a kind of database or container class. and we want to query it for objects with certain properties. in sane languages (supporting polymorphy) you could write that as <container variable>.find_board(cbname). of course "match" would work too, but as explained above i would like to use different terms just to have a (mental) distinction. > > >>> aaaand if we change board_match_pci_card_ids we should also change > >>> pci_card_find to pci_dev_find or something like that... :) > >>> > >>> > >> Well, if we rename that one, we'd have to call it pci_dev_subsys_find, > >> and that's a net loss from the 80 column perspective, but it may indeed > >> clarify the code. Further input is appreciated. > >> > > why do we have to? we find pci devs, not pci dev subsystems ;) > > hm maybe it should be find_pci_dev? > > > > pci_dev_find() looks for a device matching the supplied vendor+dev IDs. > pci_card_find() looks for a device matching the supplied > vendor+dev+subvendor+subdev IDs. > > The name "card" was picked to allow differentiating between PCI devices > (cards) which share vendor+dev ID, but have different subvendor+subdev > IDs. That's pretty common for network cards where dozens of vendors use > the same network chip (and thus fixed vendor+dev ID), but completely > different PCBs. hm, ok well... it is certainly an improvement over the current state and this discussion wont lead to something more sane i feel, so think of it as Acked-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Am 21.08.2011 21:50 schrieb Stefan Tauner: > On Tue, 16 Aug 2011 13:38:05 +0200 > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > >> Am 15.08.2011 23:42 schrieb Stefan Tauner: >> >>> oh my. now that i see the code, i think renaming the board enables to >>> board_match is really a bad idea. mainly because we use "match" as verb >>> in other places, but here as substantive. >>> e.g. >>> static const struct board_match *board_match_cbname >>> a function that matches boards according to their cbname should return >>> a board, not a "board match". it is just a question of taste, but i >>> find it awful :) >>> >> It's a trap! >> The struct is there to match a board, so struct board_match is exactly >> the right name for the _type_. >> > if that would be its only purpose, it would contain identification > attributes only. last time i looked it had general info about boards > (vendor and board name) and board enable specific fields (phase, > max_rom etc). its previous name also suggests it is not just for > matching. ;) > Hm right. >>> the wiki table is named board_info hmhm maybe board_detail, but that's >>> long.. :/ >>> what about just "board"? >>> another way to mitigate "my" problem would be to no use match as a verb >>> for the method names, but using "get" or "find" instead. >>> >> "get" has the wrong semantic implication for the cbname matching, and >> "find" as in board_find_cbname suffers from the same problem because >> we're NOT interested in finding the cbname of the board, but rather in >> looking for a board which matches the supplied cbname. >> > i think the reason for our difference of opinion is how we view the > table. for me it is a kind of database or container class. and we want > to query it for objects with certain properties. > > in sane languages (supporting polymorphy) you could write that as > <container variable>.find_board(cbname). of course "match" would work > too, but as explained above i would like to use different terms just to > have a (mental) distinction. > Understood, your view makes sense. To be honest, my approach to renaming functions is "would I misunderstand the purpose of the function?" >> pci_dev_find() looks for a device matching the supplied vendor+dev IDs. >> pci_card_find() looks for a device matching the supplied >> vendor+dev+subvendor+subdev IDs. >> >> The name "card" was picked to allow differentiating between PCI devices >> (cards) which share vendor+dev ID, but have different subvendor+subdev >> IDs. That's pretty common for network cards where dozens of vendors use >> the same network chip (and thus fixed vendor+dev ID), but completely >> different PCBs. >> > hm, ok > > well... it is certainly an improvement over the current state and this > discussion wont lead to something more sane i feel, so think of it as > Acked-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> Thanks, committed in r1424. Regards, Carl-Daniel
Patch
Index: flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/print_wiki.c (Arbeitskopie) @@ -125,7 +125,7 @@ int num_notes = 0; char *notes = calloc(1, 1); char tmp[900 + 1]; - const struct board_pciid_enable *b = board_pciid_enables; + const struct board_match *b = board_matches; for (i = 0; boards[i].vendor != NULL; i++) { if (boards[i].working) Index: flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/flashrom.c (Arbeitskopie) @@ -1731,7 +1731,7 @@ msg_gerr("Chipset enables table does not exist!\n"); ret = 1; } - if (board_pciid_enables == NULL) { + if (board_matches == NULL) { msg_gerr("Board enables table does not exist!\n"); ret = 1; } Index: flashrom-cosmetic_shorten_boardenable_function_names/programmer.h =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/programmer.h (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/programmer.h (Arbeitskopie) @@ -158,7 +158,7 @@ P3 }; -struct board_pciid_enable { +struct board_match { /* Any device, but make it sensible, like the ISA bridge. */ uint16_t first_vendor; uint16_t first_device; @@ -190,7 +190,7 @@ int (*enable) (void); /* May be NULL. */ }; -extern const struct board_pciid_enable board_pciid_enables[]; +extern const struct board_match board_matches[]; struct board_info { const char *vendor; Index: flashrom-cosmetic_shorten_boardenable_function_names/print.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/print.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/print.c (Arbeitskopie) @@ -204,7 +204,7 @@ const char *devicetype) { int i, boardcount_good = 0, boardcount_bad = 0; - const struct board_pciid_enable *e = board_pciid_enables; + const struct board_match *e = board_matches; const struct board_info *b = boards; int maxvendorlen = strlen("Vendor") + 1; int maxboardlen = strlen("Board") + 1; @@ -242,7 +242,7 @@ msg_ginfo(" "); msg_ginfo((b->working) ? "OK " : "BAD "); - for (e = board_pciid_enables; e->vendor_name != NULL; e++) { + for (e = board_matches; e->vendor_name != NULL; e++) { if (strcmp(e->vendor_name, b->vendor) || strcmp(e->board_name, b->name)) continue; Index: flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c =================================================================== --- flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c (Revision 1413) +++ flashrom-cosmetic_shorten_boardenable_function_names/board_enable.c (Arbeitskopie) @@ -1971,7 +1971,7 @@ */ /* Please keep this list alphabetically ordered by vendor/board name. */ -const struct board_pciid_enable board_pciid_enables[] = { +const struct board_match board_matches[] = { /* first pci-id set [4], second pci-id set [4], dmi identifier, coreboot id [2], phase, vendor name, board name max_rom_... OK? flash enable */ #if defined(__i386__) || defined(__x86_64__) @@ -2097,11 +2097,11 @@ * Match boards on coreboot table gathered vendor and part name. * Require main PCI IDs to match too as extra safety. */ -static const struct board_pciid_enable *board_match_coreboot_name( - const char *vendor, const char *part) +static const struct board_match *board_match_cbname(const char *vendor, + const char *part) { - const struct board_pciid_enable *board = board_pciid_enables; - const struct board_pciid_enable *partmatch = NULL; + const struct board_match *board = board_matches; + const struct board_match *partmatch = NULL; for (; board->vendor_name; board++) { if (vendor && (!board->lb_vendor @@ -2148,12 +2148,11 @@ /* * Match boards on PCI IDs and subsystem IDs. - * Second set of IDs can be main only or missing completely. + * Second set of IDs can be either main+subsystem IDs, main IDs or no IDs. */ -const static struct board_pciid_enable *board_match_pci_card_ids( - enum board_match_phase phase) +const static struct board_match *board_match_pci_ids(enum board_match_phase phase) { - const struct board_pciid_enable *board = board_pciid_enables; + const struct board_match *board = board_matches; for (; board->vendor_name; board++) { if ((!board->first_card_vendor || !board->first_card_device) && @@ -2199,7 +2198,7 @@ return NULL; } -static int unsafe_board_handler(const struct board_pciid_enable *board) +static int unsafe_board_handler(const struct board_match *board) { if (!board) return 1; @@ -2226,9 +2225,9 @@ /* FIXME: Should this be identical to board_flash_enable? */ static int board_handle_phase(enum board_match_phase phase) { - const struct board_pciid_enable *board = NULL; + const struct board_match *board = NULL; - board = board_match_pci_card_ids(phase); + board = board_match_pci_ids(phase); if (unsafe_board_handler(board)) board = NULL; @@ -2257,14 +2256,14 @@ int board_flash_enable(const char *vendor, const char *part) { - const struct board_pciid_enable *board = NULL; + const struct board_match *board = NULL; int ret = 0; if (part) - board = board_match_coreboot_name(vendor, part); + board = board_match_cbname(vendor, part); if (!board) - board = board_match_pci_card_ids(P3); + board = board_match_pci_ids(P3); if (unsafe_board_handler(board)) board = NULL;