Re: [Qemu-devel] Re: [PATCH 09/15] eepro100: convert to new pci interface

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> 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.

[...]