Re: [PATCH 1/2] d3drm: Implement IDirect3DRM*::CreateObject (v2).
On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c index a8f9c40..2b0174d 100644 --- a/dlls/d3drm/tests/d3drm.c +++ b/dlls/d3drm/tests/d3drm.c @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int test_idx, REFCLSID clsid, REFIID context.test_idx = test_idx; context.obj = obj;
- hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context)) != D3DRMERR_BADVALUE) + return; I don't think we want that. Doing this means we won't notice when the function starts returning the wrong value, and the following tests get effectively disabled too in that case.
+ hr = IDirect3DRM_CreateObject(d3drm1, &CLSID_DirectDraw, NULL, &IID_IDirectDraw, (void **)&unknown); + ok(hr == CLASSFACTORY_E_FIRST, "Expected hr == CLASSFACTORY_E_FIRST, got %#x.\n", hr); + ok(!unknown, "Expected object returned == NULL, got %p.\n", unknown); It would be better to initialise "unknown" with some known value before calling CreateObject(). Like this, if CreateObject() doesn't touch the value, the test may randomly pass or fail depending on what the initial value of "unknown" ends up being.
Sorry for the late reply, I was sick for the last couple of days. On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c index a8f9c40..2b0174d 100644 --- a/dlls/d3drm/tests/d3drm.c +++ b/dlls/d3drm/tests/d3drm.c @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int test_idx, REFCLSID clsid, REFIID context.test_idx = test_idx; context.obj = obj;
- hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context)) != D3DRMERR_BADVALUE) + return; I don't think we want that. Doing this means we won't notice when the function starts returning the wrong value, and the following tests get effectively disabled too in that case.
I do agree, but what should we do when a new object is implemented? If it's okay to extend CreateObject to include the new interface as well as implement Add/Delete destroy callbacks in the same patch, then there's no problem going with this. I had intended to keep the return statement in case a new object was added into CreateObject, but the destroy callback functions for that object weren't implemented.
+ hr = IDirect3DRM_CreateObject(d3drm1, &CLSID_DirectDraw, NULL, &IID_IDirectDraw, (void **)&unknown); + ok(hr == CLASSFACTORY_E_FIRST, "Expected hr == CLASSFACTORY_E_FIRST, got %#x.\n", hr); + ok(!unknown, "Expected object returned == NULL, got %p.\n", unknown); It would be better to initialise "unknown" with some known value before calling CreateObject(). Like this, if CreateObject() doesn't touch the value, the test may randomly pass or fail depending on what the initial value of "unknown" ends up being.
Right, I'll resend the patch with the changes right away.
Cheers, Aaryaman
On 20 April 2016 at 16:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
Sorry for the late reply, I was sick for the last couple of days.
On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c index a8f9c40..2b0174d 100644 --- a/dlls/d3drm/tests/d3drm.c +++ b/dlls/d3drm/tests/d3drm.c @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int test_idx, REFCLSID clsid, REFIID context.test_idx = test_idx; context.obj = obj;
- hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context)) != D3DRMERR_BADVALUE) + return; I don't think we want that. Doing this means we won't notice when the function starts returning the wrong value, and the following tests get effectively disabled too in that case.
I do agree, but what should we do when a new object is implemented? If it's okay to extend CreateObject to include the new interface as well as implement Add/Delete destroy callbacks in the same patch, then there's no problem going with this. I had intended to keep the return statement in case a new object was added into CreateObject, but the destroy callback functions for that object weren't implemented. The easiest way would be to just send the patch that implements those first. The slightly more complicated way would be to add another todo flag.
Keeping it simple by using the easiest way would be better I think. Besides the callback functions all behave the same regardless of the object it was called on, according to the tests. Cheers, Aaryaman On Wed, Apr 20, 2016 at 7:34 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 20 April 2016 at 16:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com> wrote:
Sorry for the late reply, I was sick for the last couple of days.
On Tue, Apr 19, 2016 at 6:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
On 18 April 2016 at 19:00, Aaryaman Vasishta <jem456.vasishta(a)gmail.com
wrote:
diff --git a/dlls/d3drm/tests/d3drm.c b/dlls/d3drm/tests/d3drm.c index a8f9c40..2b0174d 100644 --- a/dlls/d3drm/tests/d3drm.c +++ b/dlls/d3drm/tests/d3drm.c @@ -1269,7 +1269,8 @@ static void test_destroy_callback(unsigned int test_idx, REFCLSID clsid, REFIID context.test_idx = test_idx; context.obj = obj;
- hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context); + if ((hr = IDirect3DRMObject_AddDestroyCallback(obj, NULL, &context)) != D3DRMERR_BADVALUE) + return; I don't think we want that. Doing this means we won't notice when the function starts returning the wrong value, and the following tests get effectively disabled too in that case.
I do agree, but what should we do when a new object is implemented? If it's okay to extend CreateObject to include the new interface as well as implement Add/Delete destroy callbacks in the same patch, then there's no problem going with this. I had intended to keep the return statement in case a new object was added into CreateObject, but the destroy callback functions for that object weren't implemented. The easiest way would be to just send the patch that implements those first. The slightly more complicated way would be to add another todo flag.
participants (2)
-
Aaryaman Vasishta -
Henri Verbeet