Re: [Qemu-devel] [PATCH 6/7] mac_dbdma: change DBDMA_register_channel to a MAC_DBDMA type method

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 1749 bytes --]

On Thu, Sep 28, 2017 at 07:40:18AM +0100, Mark Cave-Ayland wrote:
> On 26/09/17 04:47, David Gibson wrote:
> 
> > On Sun, Sep 24, 2017 at 03:47:45PM +0100, Mark Cave-Ayland wrote:
> >> Using this we can change the MACIO_IDE instance to register the channel
> >> itself via a type method instead of requiring a separate
> >> DBDMA_register_channel() function.
> >>
> >> As a consequence of this it is now possible to remove the old external
> >> macio_ide_register_dma() function.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > 
> > Ok, two concerns about this.
> > 
> > First, you've added the function pointer to the instance, not to the
> > class, which is not how a QOM method would normally be done.
> 
> Yeah I did think about whether I needed to create a full class but was
> torn since as you say there is only one implementation.
> 
> > More generally, though, why is a method preferable to a plain
> > function?  AFAICT it's not plausible that there will ever be more than
> > one implementation of the method.
> > 
> > Same comments apply to patch 7/7.
> 
> For me it's really for encapsulation. It seems a little odd requiring a
> global function to configure a QOM object to which I already have a
> reference.

Instead you're using the method which is defined in a global type
definition.  I don't think it really makes any different to
encapsulation.

> If I were to redo the last 2 patches using a proper class, would you
> accept them?
> 
> 
> ATB,
> 
> Mark.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]