RE: [PATCH 1/3] d3drm/tests: Add test for IDirect3DRM*::CreateDeviceFromClipper (try 5).
This is by no means a full review, just a couple of obvious things I noticed.
On 24 June 2015 at 17:02, Aaryaman Vasishta <jem456.vasishta at gmail.com <https://www.winehq.org/mailman/listinfo/wine-devel>> wrote:
* +static HRESULT CALLBACK surface_callback(IDirectDrawSurface *surface, DDSURFACEDESC *desc, void *context) *>* +{ *>* + IDirectDrawClipper *d3drm_clipper = NULL; *Initializing this to NULL is pointless, you initialize it before all uses with the IDirectDrawSurface_GetClipper() call below.
* + if (context) *>* + { *"context" should always be non-NULL here.
* + hr = IDirectDrawSurface_GetClipper(surface, &d3drm_clipper); *>* + ok(hr == DD_OK, "Cannot get attached clipper from primary surface (hr = %x).\n", hr); *>* + if (SUCCEEDED(hr)) *You have an ok() one line above that states "hr == DD_OK" at this point. If the GetClipper() call failed you'd get a test failure.
Once you've found the primary you can of course return DDENUMRET_CANCEL.
I think you're making things too complicated though. I'd just call EnumSurfaces() with "&primary" as context pointer, set it in the callback, and then just do the rest of the tests after EnumSurfaces() returns, instead of trying to fit those into the callback.
Thank you for the suggestions! I have decided to return the primary
surface directly, and set it to NULL if it doesn't exist via the context, something like this, which seems to run fine on my windows machine:
static HRESULT CALLBACK surface_callback(IDirectDrawSurface *surface, DDSURFACEDESC *desc, void **context) { IDirectDrawClipper *d3drm_clipper; HRESULT hr;
if (desc->ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE) { *context = surface; return DDENUMRET_CANCEL; } IDirectDrawSurface_Release(surface);
return DDENUMRET_OK; }
Then in the tests, simply check if the context returned is NULL, if yes then perform related tests on it inside the test. Thank you!
That d3drm_clipper variable is pointless, but please let me know if the rest of it is fine. Thank you! Jam On Fri, Jun 26, 2015 at 1:41 AM, Aaryaman Vasishta < jem456.vasishta(a)gmail.com> wrote:
This is by no means a full review, just a couple of obvious things I noticed.
On 24 June 2015 at 17:02, Aaryaman Vasishta <jem456.vasishta at gmail.com <https://www.winehq.org/mailman/listinfo/wine-devel>> wrote:
* +static HRESULT CALLBACK surface_callback(IDirectDrawSurface *surface, DDSURFACEDESC *desc, void *context) *>* +{ *>* + IDirectDrawClipper *d3drm_clipper = NULL; *Initializing this to NULL is pointless, you initialize it before all uses with the IDirectDrawSurface_GetClipper() call below.
* + if (context) *>* + { *"context" should always be non-NULL here.
* + hr = IDirectDrawSurface_GetClipper(surface, &d3drm_clipper); *>* + ok(hr == DD_OK, "Cannot get attached clipper from primary surface (hr = %x).\n", hr); *>* + if (SUCCEEDED(hr)) *You have an ok() one line above that states "hr == DD_OK" at this point. If the GetClipper() call failed you'd get a test failure.
Once you've found the primary you can of course return DDENUMRET_CANCEL.
I think you're making things too complicated though. I'd just call EnumSurfaces() with "&primary" as context pointer, set it in the callback, and then just do the rest of the tests after EnumSurfaces() returns, instead of trying to fit those into the callback.
Thank you for the suggestions! I have decided to return the primary
surface directly, and set it to NULL if it doesn't exist via the context, something like this, which seems to run fine on my windows machine:
static HRESULT CALLBACK surface_callback(IDirectDrawSurface *surface, DDSURFACEDESC *desc, void **context) { IDirectDrawClipper *d3drm_clipper; HRESULT hr;
if (desc->ddsCaps.dwCaps & DDSCAPS_PRIMARYSURFACE) { *context = surface; return DDENUMRET_CANCEL; } IDirectDrawSurface_Release(surface);
return DDENUMRET_OK; }
Then in the tests, simply check if the context returned is NULL, if yes then perform related tests on it inside the test.
Thank you!
participants (1)
-
Aaryaman Vasishta