Re: dsound: Avoid using the COM method implementations directly.
I guess I'm not opposed to this, but are these changes useful? Andrew On Fri, Sep 02, 2016 at 10:37:33AM +0200, Michael Stefaniuc wrote:
Signed-off-by: Michael Stefaniuc <mstefani(a)redhat.de> --- dlls/dsound/capture.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/dsound/capture.c b/dlls/dsound/capture.c index 30870ae..38e3045 100644 --- a/dlls/dsound/capture.c +++ b/dlls/dsound/capture.c @@ -861,7 +861,7 @@ static ULONG DirectSoundCaptureDevice_Release( LeaveCriticalSection(&DSOUND_capturers_lock);
if (device->capture_buffer) - IDirectSoundCaptureBufferImpl_Release(&device->capture_buffer->IDirectSoundCaptureBuffer8_iface); + IDirectSoundCaptureBuffer8_Release(&device->capture_buffer->IDirectSoundCaptureBuffer8_iface);
if(device->mmdevice) IMMDevice_Release(device->mmdevice); -- 2.7.4
On 2 September 2016 at 19:17, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
I guess I'm not opposed to this, but are these changes useful?
It makes the code harder to follow and will generate worse code, if perhaps both only slightly. That's perhaps not in itself a reason not to do it, but I do think it makes it harder to justify. I'd be against this for any code I maintain.
On 09/02/2016 07:17 PM, Andrew Eikum wrote:
I guess I'm not opposed to this, but are these changes useful? It makes the code consistent aka one way to call the COM methods. There were also forward declarations to use the methods directly but it looks like I have removed those during the COM cleanup.
Anyway with very few exceptions (ddraw, d3d11, ole32, urlmon32) there is just a handful, most of the times just a single, direct method use per dll: d2d1/geometry.c | 4 +-- d3d11/device.c | 14 ++++++------- ddraw/surface.c | 44 +++++++++++++++++++++---------------------- devenum/mediacatenum.c | 2 - dinput/effect_linuxinput.c | 6 ++--- dmime/performance.c | 2 - dmusic/clock.c | 2 - dmusic/dmusic.c | 2 - dpnet/address.c | 2 - dpnet/client.c | 4 +-- dsound/capture.c | 2 - inetcomm/mimeole.c | 2 - msctf/threadmgr.c | 8 +++---- mshtml/htmlwindow.c | 2 - objsel/objsel.c | 4 +-- ole32/antimoniker.c | 2 - ole32/bindctx.c | 2 - ole32/comcat.c | 2 - ole32/compositemoniker.c | 2 - ole32/datacache.c | 2 - ole32/itemmoniker.c | 2 - ole32/moniker.c | 2 - ole32/pointermoniker.c | 2 - oleaut32/tests/olepicture.c | 2 - oleaut32/tmarshal.c | 2 - oleaut32/typelib.c | 4 +-- quartz/enummoniker.c | 2 - quartz/systemclock.c | 2 - shlwapi/tests/clist.c | 4 +-- urlmon/uri.c | 12 +++++------ windowscodecs/propertybag.c | 2 - winealsa.drv/mmdevdrv.c | 4 +-- winecoreaudio.drv/mmdevdrv.c | 4 +-- wineoss.drv/mmdevdrv.c | 4 +-- winepulse.drv/mmdevdrv.c | 4 +-- 35 files changed, 81 insertions(+), 81 deletions(-) But of course it is a matter of taste. bye michael
On 5 September 2016 at 09:39, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/02/2016 07:17 PM, Andrew Eikum wrote:
I guess I'm not opposed to this, but are these changes useful? It makes the code consistent aka one way to call the COM methods.
I'm all for consistency, but these do different things. One calls a method from an interface that may have different implementations, and the other calls a specific function/implementation. Other than taking an interface pointer as the first argument, the latter doesn't have a lot to do with COM at all. You could make that more explicit by introducing a separate function that takes an implementation pointer instead, but in most cases I don't think that's worth it.
On 09/05/2016 09:50 AM, Henri Verbeet wrote:
On 5 September 2016 at 09:39, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/02/2016 07:17 PM, Andrew Eikum wrote:
I guess I'm not opposed to this, but are these changes useful? It makes the code consistent aka one way to call the COM methods.
I'm all for consistency, but these do different things. One calls a method from an interface that may have different implementations, and the other calls a specific function/implementation. Other than taking Right, and by using the COM macro it is always guaranteed that the correct method implementation is used.
And by consistent I mean that we don't have IFooImpl_AddRef() paired with IFoo_Release() like in the bulk of diff that my script generated.
an interface pointer as the first argument, the latter doesn't have a lot to do with COM at all. You could make that more explicit by introducing a separate function that takes an implementation pointer instead, but in most cases I don't think that's worth it.
bye michael
On 5 September 2016 at 10:04, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/05/2016 09:50 AM, Henri Verbeet wrote:
On 5 September 2016 at 09:39, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/02/2016 07:17 PM, Andrew Eikum wrote:
I guess I'm not opposed to this, but are these changes useful? It makes the code consistent aka one way to call the COM methods.
I'm all for consistency, but these do different things. One calls a method from an interface that may have different implementations, and the other calls a specific function/implementation. Other than taking Right, and by using the COM macro it is always guaranteed that the correct method implementation is used. At the cost of making it less obvious what that implementation actually is. I'll also note that for ddraw in particular it isn't necessarily always safe to change from one variant to the other. There are applications that modify the call tables to install their own hooks, and picking one variant or the other would make those hooks get called or not for internal calls.
And by consistent I mean that we don't have IFooImpl_AddRef() paired with IFoo_Release() like in the bulk of diff that my script generated.
If a large part of the diff is in D3D code, I'd question whether that's really the bulk of the diff. But sure, that's awkward. If you have an implementation pointer I'd argue you should just use IFooImpl_Release() as well in those cases though.
On 09/05/2016 11:54 AM, Henri Verbeet wrote:
On 5 September 2016 at 10:04, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/05/2016 09:50 AM, Henri Verbeet wrote:
On 5 September 2016 at 09:39, Michael Stefaniuc <mstefani(a)redhat.com> wrote:
On 09/02/2016 07:17 PM, Andrew Eikum wrote:
I guess I'm not opposed to this, but are these changes useful? It makes the code consistent aka one way to call the COM methods.
I'm all for consistency, but these do different things. One calls a method from an interface that may have different implementations, and the other calls a specific function/implementation. Other than taking Right, and by using the COM macro it is always guaranteed that the correct method implementation is used. At the cost of making it less obvious what that implementation actually is. I'll also note that for ddraw in particular it isn't necessarily always safe to change from one variant to the other. There are applications that modify the call tables to install their own hooks, and picking one variant or the other would make those hooks get called or not for internal calls.
And by consistent I mean that we don't have IFooImpl_AddRef() paired with IFoo_Release() like in the bulk of diff that my script generated.
If a large part of the diff is in D3D code, I'd question whether that's really the bulk of the diff. But sure, that's awkward. If you I have mentally excluded ddraw as that stands out. The bulk of the patches I would submit would just fix a "dangling" AddRef or Release. Some of those look like copy and paste from the wrong example code than any deeper reason.
have an implementation pointer I'd argue you should just use IFooImpl_Release() as well in those cases though.
bye michael
participants (3)
-
Andrew Eikum -
Henri Verbeet -
Michael Stefaniuc