Attention is currently required from: Edward O'Callaghan.

View Change

1 comment:

• File board_enable.c:

  • Patch Set #3, Line 1097: dev = pcidev_clonedev(dev);

    Yes `pacc` is a handle scoped to libpci and so lexically scope to pcidev.c which is what this change is doing.

    I see what your code is doing, but I don't know your reasoning. Are you just
    following design patterns you have learned or did you think it through? how
    should I know? That's why we have a mailing list where we can discuss such
    design choices. If we don't discuss such things and the reasons are not 100%
    obvious, another developer with a different opinion might accidently work
    on reverting your changes in the future. I've seen this a lot in coreboot
    btw. it's not just a figment.

    Currently `pacc` is global yes. But as you well know we want to get rid of
    global state. So eventually, `pacc` might just become another, locally known
    parameter, like `dev` is to pci_read_long() etc. And I don't see (yet), that
    we'd be wrapping every single libpci function in `pcidev.c`. That's not saying
    that it's wrong to do so for some functions, I just think our reasoning should
    be more subtle.

    Also, after reading above paragraphs together again: Can we tell that this
    change makes replacing the global `pacc` easier or harder? That's all things
    a reviewer must consider when the direction is unclear.

To view, visit change 59276. To unsubscribe, or for help writing mail filters, visit settings.