On Wed, Feb 10, 2010 at 11:43:29AM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <m...@redhat.com> writes:
>
> > On Wed, Feb 10, 2010 at 07:32:10AM +0100, Stefan Weil wrote:
> >> See my inline comments.
> >>
> >>
> >> Anthony Liguori schrieb:
> [...]
> >> > -static void pci_map(PCIDevice * pci_dev, int region_num,
> >> > - pcibus_t addr, pcibus_t size, int type)
> >> > +static void pci_io_write(PCIDevice *dev, pcibus_t addr, int size,
> >> > + uint32_t value)
> >> > {
> >> > - EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, pci_dev);
> >> > -
> >> > - TRACE(OTHER, logout("region %d, addr=0x%08"FMT_PCIBUS", "
> >> > - "size=0x%08"FMT_PCIBUS", type=%d\n",
> >> > - region_num, addr, size, type));
> >> > + EEPRO100State *s = DO_UPCAST(EEPRO100State, dev, dev);
> >> >
> >>
> >> Please don't change the name of the PCIDevice pointer argument
> >> from pci_dev to dev.
> >>
> >> This dev, dev in DO_UPCAST is ugly and misleading.
> >
> > It's a matter of taste: I actually think it's pretty: I never remember
> > which argument of DO_UPCAST is which, and if both are dev it doesn't
> > matter.
>
> I also have trouble remembering how to use DO_UPCAST(), but playing
> games with identifiers is a poor fix for that. I find the use of the
> same name "dev" for two different things in the same expression rather
> confusing. Could we improve DO_UPCAST() instead?
>
> For what it's worth, I have no trouble with container_of(). DO_UPCAST()
> takes the same arguments in a different order, which I find irritating.
>
> [...]
IMO it would be ideal to remove offset assumptions where possible
and then we can use container_of.
--
MST
