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.
Hi Matteo,
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.
The problem I had with it is actually when ok(...) with the call to this function as the condition fails within the main test function body, I get the line of failure inside this function (and not of the actual ok()), which is confusing. I didn't check, it may be related to compiler optimization/inlining somehow and may be not reproducible with default build flags, but should not I avoid that? Maybe I would better trace failure messages passing a main test context string?
- 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.
I could not find two consequent BeginPass() tests, I can add some as a separate case, but do you think it is related here to the out of bounds behaviour? BTW if I add EndPass right after the first (failing) BeginPass here, it will fail as expected.
What is shown here, is that after setting variable with Set function, the first call to BeginPass fails, while exactly the same next one succeeds, which suggests that it is related to the "dirty" parameter state which is reset within even an unsuccessful call to BeginPass. The 2nd BeginPass selects the last element in the array for both negative and positive out of bounds index. But there is a special value of -1 for index, which makes even the first call to BeginPass succeed and select first array element. At the same time, CommitChanges never fails, while the state is not updated. When you say what happens with the same or different pass index, do you mean a test where the same variable causes out of bounds behaviour in 2 different passes, and check if BeginPass() will succeed for another pass after failing for the first one? This will require blob modification, as currently 2 passes have array selectors, but different variables are used there. Maybe add a completely different test blob in this case not to overload & constantly update all the same big one?
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.
The motivation under this test was that I started preparing my patches for dirty parameter flags update and usage in CommitChanges, and while extending the test I found that very specific behaviour of out of bounds access processing. So I extracted this part of test into separate one to split the thing. I actually suggest not to cover this todo's in the initial dirty parameters update implementation leaving the out of bounds processing in some consistent state, i. e., always clamp to range or always skip the state (as it is done now) without failing BeginPass().
BTW, I think it's somewhat idiomatic to call it "out of bounds" even when technically there is a single bound involved.
I will rename it.
2017-03-07 20:57 GMT+01:00 Paul Gofman gofmanp@gmail.com:
Hi Matteo,
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.
The problem I had with it is actually when ok(...) with the call to this function as the condition fails within the main test function body, I get the line of failure inside this function (and not of the actual ok()), which is confusing. I didn't check, it may be related to compiler optimization/inlining somehow and may be not reproducible with default build flags, but should not I avoid that? Maybe I would better trace failure messages passing a main test context string?
Not sure I understand your issue. I don't think there is any problem if the ok() call prints the line in the helper vs the line in the caller function. If you want to "fix" that you can, e.g. look at check_srv_desc() in d3d11/tests/d3d11.c for a possible solution. I don't think it's necessary here though.
- 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.
I could not find two consequent BeginPass() tests, I can add some as a separate case, but do you think it is related here to the out of bounds behaviour?
No, that was an idea for a separate patch.
BTW if I add EndPass right after the first (failing) BeginPass here, it will fail as expected.
When you say what happens with the same or different pass index, do youmean a test where the same variable causes out of bounds behaviour in 2 different passes, and check if BeginPass() will succeed for another pass after failing for the first one?
No, just a BeginPass(..., 0) followed by BeginPass(..., 1) or something like that, with no EndPass() in between. Again, ideas for another patch.
The motivation under this test was that I started preparing my patches for dirty parameter flags update and usage in CommitChanges, and while extending the test I found that very specific behaviour of out of bounds access processing. So I extracted this part of test into separate one to split the thing. I actually suggest not to cover this todo's in the initial dirty parameters update implementation leaving the out of bounds processing in some consistent state, i. e., always clamp to range or always skip the state (as it is done now) without failing BeginPass().
Sure, that's okay. Whatever is less annoying for you when implementing the dirty state tracking.
Thank you, I will update the patch with helper function ok() calls and will keep in mind to add more tests around multiple passes (including those you mention here) to dirty state update tests.
On 03/07/2017 11:43 PM, Matteo Bruni wrote:
2017-03-07 20:57 GMT+01:00 Paul Gofman gofmanp@gmail.com:
Hi Matteo,
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.
The problem I had with it is actually when ok(...) with the call to this function as the condition fails within the main test function body, I get the line of failure inside this function (and not of the actual ok()), which is confusing. I didn't check, it may be related to compiler optimization/inlining somehow and may be not reproducible with default build flags, but should not I avoid that? Maybe I would better trace failure messages passing a main test context string?
Not sure I understand your issue. I don't think there is any problem if the ok() call prints the line in the helper vs the line in the caller function. If you want to "fix" that you can, e.g. look at check_srv_desc() in d3d11/tests/d3d11.c for a possible solution. I don't think it's necessary here though.
- 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.
I could not find two consequent BeginPass() tests, I can add some as a separate case, but do you think it is related here to the out of bounds behaviour?
No, that was an idea for a separate patch.
BTW if I add EndPass right after the first (failing) BeginPass here, it will fail as expected.
When you say what happens with the same or different pass index, do youmean a test where the same variable causes out of bounds behaviour in 2 different passes, and check if BeginPass() will succeed for another pass after failing for the first one?
No, just a BeginPass(..., 0) followed by BeginPass(..., 1) or something like that, with no EndPass() in between. Again, ideas for another patch.
The motivation under this test was that I started preparing my patches for dirty parameter flags update and usage in CommitChanges, and while extending the test I found that very specific behaviour of out of bounds access processing. So I extracted this part of test into separate one to split the thing. I actually suggest not to cover this todo's in the initial dirty parameters update implementation leaving the out of bounds processing in some consistent state, i. e., always clamp to range or always skip the state (as it is done now) without failing BeginPass().
Sure, that's okay. Whatever is less annoying for you when implementing the dirty state tracking.
-
Matteo Bruni -
Paul Gofman