November 2014 - Wine-Devel - list.winehq.org

Re: [1/2] d3dx9_36: Implement D3DXCreatePolygon. (try 5)

by Matteo Bruni

2014-11-05 18:47 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: > Argh, head->wall - accidentially made the changes to v2 instead of v3. ;) > Again replace ~0 -> ~0U. Yeah, I noticed that but it wasn't a showstopper of course, no need to bang your head to the wall :D I confirm my approval (as much as it might matter), including patch 107477. > > Based on a patch by David Adam. > --- > dlls/d3dx9_36/d3dx9_36.spec | 2 +- > dlls/d3dx9_36/mesh.c | 88 +++++++++++++++++++++++++++++++++++++++++++++ > include/d3dx9shape.h | 2 ++ > 3 files changed, 91 insertions(+), 1 deletion(-) > > >

Nov. 5, 2014

Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 4)

by Matteo Bruni

2014-11-05 18:25 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: > Changes in try 4: > * Use smaller buffer > * Fix typo > * Remove error checking & goto > > Changes in try 3: > * Replace ~0 -> ~0U. > > Changes in try 2: > * Lot of style cleanups - hopefully I didn't miss anything. > * Fix condition for 'sides' argument (should be >= 3, not just != 0) > * Added test to check if *mesh is cleared on error - tests show that this is not the case. > I only kept a single test for that to avoid making the test code too ugly. > > Based on a patch by David Adam. > --- > dlls/d3dx9_36/tests/mesh.c | 155 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 155 insertions(+) FWIW, series looks good to me.

Nov. 5, 2014

Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

by Matteo Bruni

2014-11-05 18:30 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: > I am also fine with fixing these issues right away, especially because of the ugly typo - the error check for D3DXCreatePolygon() was copied from existing tests, I assumed there is a good reason why D3DXCreate*() could fail on some machines/testbots. I think that kind of check only makes sense if you write the test before the implementation, in that case you need something like that to avoid the crash when running the test with builtin d3dx9. I haven't looked but probably the patch implementing the function removed the todo_wine from the test but forgot to remove the if. At least, I can't think of any other reason for that to fail in the test. > Would you mind taking another look please? Thanks for your feedback! Sure, going to take a look. > Regards, > Sebastian > > On 05.11.2014 18:03, Matteo Bruni wrote: >> 2014-11-04 19:57 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: >>> Based on a patch by David Adam. >>> --- >>> dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 163 insertions(+) >> >> Hi Sebastian, >> >> first of all, let me say I'm okay with these patches going in as they are. >> >> IMHO there is still room for improvement though: >> >> +static void test_polygon(IDirect3DDevice9 *device, float length, >> unsigned int sides) >> +{ >> + HRESULT hr; >> + ID3DXMesh *polygon; >> + struct mesh mesh; >> + char name[256]; >> >> No need to use such a big array there. >> >> + hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL); >> + ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr); >> + if (hr != D3D_OK) >> + { >> + skip("Couldn't create box\n"); >> >> Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages. >> >> + adjacency = NULL; >> + hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency); >> + ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr); >> + >> + if (FAILED(hr)) >> + { >> + skip("D3DXCreatePolygon failed\n"); >> + goto end; >> + } >> >> That D3DXCreatePolygon() call is not expected to ever fail, so no need >> for the "if". >> >> + if (adjacency) >> + ID3DXBuffer_Release(adjacency); >> >> If you remove the previous "if" you won't need the "end" label and >> this one becomes always true. Then I'd put this Release() right after >> the loop checking the adjacency. >> >> Again, I don't care about those details, this is still good enough for >> me. It's mostly about any future patch you'd want to send in. >> >

Nov. 5, 2014

Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

by Sebastian Lackner

I am also fine with fixing these issues right away, especially because of the ugly typo - the error check for D3DXCreatePolygon() was copied from existing tests, I assumed there is a good reason why D3DXCreate*() could fail on some machines/testbots. Would you mind taking another look please? Thanks for your feedback! Regards, Sebastian On 05.11.2014 18:03, Matteo Bruni wrote: > 2014-11-04 19:57 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: >> Based on a patch by David Adam. >> --- >> dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 163 insertions(+) > > Hi Sebastian, > > first of all, let me say I'm okay with these patches going in as they are. > > IMHO there is still room for improvement though: > > +static void test_polygon(IDirect3DDevice9 *device, float length, > unsigned int sides) > +{ > + HRESULT hr; > + ID3DXMesh *polygon; > + struct mesh mesh; > + char name[256]; > > No need to use such a big array there. > > + hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL); > + ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr); > + if (hr != D3D_OK) > + { > + skip("Couldn't create box\n"); > > Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages. > > + adjacency = NULL; > + hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency); > + ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr); > + > + if (FAILED(hr)) > + { > + skip("D3DXCreatePolygon failed\n"); > + goto end; > + } > > That D3DXCreatePolygon() call is not expected to ever fail, so no need > for the "if". > > + if (adjacency) > + ID3DXBuffer_Release(adjacency); > > If you remove the previous "if" you won't need the "end" label and > this one becomes always true. Then I'd put this Release() right after > the loop checking the adjacency. > > Again, I don't care about those details, this is still good enough for > me. It's mostly about any future patch you'd want to send in. >

Nov. 5, 2014

Re: [PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

by Sebastian Lackner

On 05.11.2014 15:38, Alexandre Julliard wrote: > Alex Henrie <alexhenrie24(a)gmail.com> writes: > >> + SetLastError(0xdeadbeef); >> + len = WideCharToMultiByte(CP_UTF7, 0, input, 4, output, sizeof(output) - 1, NULL, NULL); >> + ok(GetLastError() == 0xdeadbeef, >> + "i=0x%04x: expected error=0xdeadbeef, got error=0x%x\n", i, GetLastError()); > > Testing last error on success is not useful and should be avoided. > Whats the reason for this Wine decision? Its not a very useful one at my opinion. Besides the fact that a lot of existing tests check that GetLastError() was unmodified (20 matches for "== 0xdeadbeef" in kernel32/tests/codepage.c), its important to know the exact behaviour when trying to propagate the error code, similar to how HeapFree(...) should keep the original error code on success. The fact that there are also API functions where the error code is indeed modified on success (for example CreateMutexExW) makes me think that such a test can basically never hurt, to avoid hunting down weird failures in the future. It would not be the first example where programs check for success the wrong way, and only look at the error code. Even if native would behave differently, and we probably would decide not to reproduce the behaviour - the tests are a good documentation of how it is supposed to work. The next person who wants to know if WideCharToMultibyte(CP_UTF7, ...) changes the error code, will have to write those tests just again. Any good reason behind this? Regards, Sebastian

Nov. 5, 2014

Re: [2/2] d3dx9_36/tests: Add tests for D3DXCreatePolygon. (try 3)

by Matteo Bruni

2014-11-04 19:57 GMT+01:00 Sebastian Lackner <sebastian(a)fds-team.de>: > Based on a patch by David Adam. > --- > dlls/d3dx9_36/tests/mesh.c | 163 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 163 insertions(+) Hi Sebastian, first of all, let me say I'm okay with these patches going in as they are. IMHO there is still room for improvement though: +static void test_polygon(IDirect3DDevice9 *device, float length, unsigned int sides) +{ + HRESULT hr; + ID3DXMesh *polygon; + struct mesh mesh; + char name[256]; No need to use such a big array there. + hr = D3DXCreatePolygon(device, length, sides, &polygon, NULL); + ok(hr == D3D_OK, "Got result %x, expected 0 (D3D_OK)\n", hr); + if (hr != D3D_OK) + { + skip("Couldn't create box\n"); Copy-paste typo. FWIW, we usually prefer a '.' at the end of trace messages. + adjacency = NULL; + hr = D3DXCreatePolygon(device, 3.0f, 11, &polygon, &adjacency); + ok(hr == D3D_OK, "Expected D3D_OK, received %#x\n", hr); + + if (FAILED(hr)) + { + skip("D3DXCreatePolygon failed\n"); + goto end; + } That D3DXCreatePolygon() call is not expected to ever fail, so no need for the "if". + if (adjacency) + ID3DXBuffer_Release(adjacency); If you remove the previous "if" you won't need the "end" label and this one becomes always true. Then I'd put this Release() right after the loop checking the adjacency. Again, I don't care about those details, this is still good enough for me. It's mostly about any future patch you'd want to send in.

Nov. 5, 2014

Re: [PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

by Alexandre Julliard

Alex Henrie <alexhenrie24(a)gmail.com> writes: > + SetLastError(0xdeadbeef); > + len = WideCharToMultiByte(CP_UTF7, 0, input, 4, output, sizeof(output) - 1, NULL, NULL); > + ok(GetLastError() == 0xdeadbeef, > + "i=0x%04x: expected error=0xdeadbeef, got error=0x%x\n", i, GetLastError()); Testing last error on success is not useful and should be avoided. -- Alexandre Julliard julliard(a)winehq.org

Nov. 5, 2014

Re: advapi32/tests: Avoid a crash on some linux hosts.

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=10035 Your paranoid android. === w7u (32 bit service) === The test timed out

Nov. 5, 2014

Re: msi: Simplify check for an empty string (PVS-Studio)

by Michael Stefaniuc

On 11/05/2014 01:26 PM, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=10031 > > Your paranoid android. > > > === w2000pro (32 bit action) === > The test timed out > > === wxppro (32 bit action) === > The test timed out > > === wvistau64 (32 bit action) === > The test timed out > > === w7u (32 bit action) === > The test timed out > > === w8 (32 bit action) === > The test timed out > > === wvistau64 (64 bit action) === > Failure running script in VM: timed out waiting for the child process msi:action is renowned at timing out: http://test.winehq.org/data/tests/msi:action.html bye michael

Nov. 5, 2014

Re: msi: Simplify check for an empty string (PVS-Studio)

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=10031 Your paranoid android. === w2000pro (32 bit action) === The test timed out === wxppro (32 bit action) === The test timed out === wvistau64 (32 bit action) === The test timed out === w7u (32 bit action) === The test timed out === w8 (32 bit action) === The test timed out === wvistau64 (64 bit action) === Failure running script in VM: timed out waiting for the child process

Nov. 5, 2014