[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [1/2] qcap: Distinguish interface and implementation pointer for VfwCapture output pin.
From:       Sebastian Lackner <sebastian () fds-team ! de>
Date:       2015-06-29 11:11:51
Message-ID: 559127F7.2040907 () fds-team ! de
[Download RAW message or body]

On 29.06.2015 12:50, Nikolay Sivov wrote:
> On 29.06.2015 13:13, Sebastian Lackner wrote:
> > Thanks for the feedback. In this case pOutputPin is basically part of the same \
> > object, not an arbitrary external interface. It is constructed with \
> > VfwPin_Construct -> BaseOutputPin_Construct, and also holds a reference to the \
> > critical section of the parent object for example, so there is no clear \
> > distinction between public/private interfaces anyway.
> > 
> > Most other code parts which use BaseOutputPin_Construct store the result in \
> > something like: (IPin **)&This->store_impl_pointer_here where the variable \
> > already has the corresponding implementation type. Should we change it here too? \
> > Using only the public interface is not possible, take a look at for example the \
> > following code part, where a private member is set from a different interface:
> > 
> > --- snip ---
> > This->driver_info = qcap_driver_init( This->pOutputPin,
> > var.__VARIANT_NAME_1.__VARIANT_NAME_2.__VARIANT_NAME_3.ulVal );
> > if (This->driver_info)
> > {
> > pin = impl_from_IPin(This->pOutputPin);
> > pin->driver_info = This->driver_info; // <---
> > --- snip ---
> 
> I personally prefer to use public interface, usually it makes things more clear, \
> and code doesn't depend on implementation changes. 
> If in this particular case it's not possible then yes, we should store impl pointer \
> instead of interface pointer.

Okay, I'll resend an updated version later then.

> 
> By the way, variant thing looks a little insane, assuming intention was to get \
> V_UI4(&var), which is not necessary right on its own as qcap_driver_init() takes \
> USHORT argument. 

Yes, I also noticed it. Changes to this part should probably be in a separate patch \
though.


[prev in list] [next in list] [prev in thread] [next in thread]