2017-03-07 18:47 GMT+01:00 Paul Gofman gofmanp@gmail.com:
Signed-off-by: Paul Gofman gofmanp@gmail.com
Hi Paul! Patch looks mostly good to me BUT I have a couple comments anyway...
dlls/d3dx9_36/tests/effect.c | 159 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 141 insertions(+), 18 deletions(-)
diff --git a/dlls/d3dx9_36/tests/effect.c b/dlls/d3dx9_36/tests/effect.c index b3845c5..fe84173 100644 --- a/dlls/d3dx9_36/tests/effect.c +++ b/dlls/d3dx9_36/tests/effect.c @@ -3813,6 +3813,33 @@ static const DWORD test_effect_preshader_effect_blob[] = #define TEST_EFFECT_PRESHADER_VSHADER_POS 2458 #define TEST_EFFECT_PRESHADER_VSHADER_LEN 13
+static BOOL test_effect_preshader_compare_shader(IDirect3DDevice9 *device, int expected_shader_index) +{
- IDirect3DVertexShader9 *vshader;
- void *byte_code;
- unsigned int byte_code_size;
- HRESULT hr;
- BOOL ret;
- if (FAILED(hr = IDirect3DDevice9_GetVertexShader(device, &vshader)) || !vshader)
return FALSE;
It seems preferable to keep the detailed ok() calls from the old test instead of a generic ok() message on the return value of test_effect_preshader_compare_shader(). I guess you want to avoid to have ambiguous ok() calls in this helper. Until we get something like the with_context() thing from https://source.winehq.org/patches/data/131137 just passing a string with the test name to the helper function and using that in the ok() messages should do the trick.
- hr = effect->lpVtbl->BeginPass(effect, 1);
- todo_wine ok(hr == E_FAIL, "Got result %#x.\n", hr);
- if (SUCCEEDED(hr))
effect->lpVtbl->EndPass(effect);- hr = effect->lpVtbl->BeginPass(effect, 1);
- ok(hr == D3D_OK, "Got result %#x.\n", hr);
That reminds me, do we have tests for two BeginPass() calls without an EndPass() in between? Might be interesting to see what happens both with the same or different pass index. Other somewhat interesting cases might be BeginPass() outside of Begin() / End() and multiple Begin() / End() / EndPass() calls in a row. It should probably be in a separate patch though.
BTW, I think it's somewhat idiomatic to call it "out of bounds" even when technically there is a single bound involved.