September 2012 - Wine-Devel

Re: Building winetest-latest.exe?

by Dan Kegel

On Tue, Sep 4, 2012 at 3:18 PM, Francois Gouget <fgouget(a)codeweavers.com> wrote: >> (But as it turns out, the code I needed to fix was in buildbot, >> not winetest.) > > Maybe the WineTestBot code needs a similar fix? > In testbot/src/TestLauncher/TestLauncher.c? Yeah, um, ^buildbot^testbot I have a patch at http://bugs.winehq.org/show_bug.cgi?id=31609 for your review.

Sept. 4, 2012

Re: Building winetest-latest.exe?

by Francois Gouget

On Mon, 3 Sep 2012, Dan Kegel wrote: [...] > > Sort of. You don't need a separate build tree for building the tests. > > Just have mingw32 installed, run ./configure, then 'make crosstest'. > > I've done 'make crosstest' many times, but building winetest.exe itself > does seem to require the two trees as described by that page. For that one can also look at testbot/bin/buil/Reconfig.pl, specifically BuildNative() and BuildCross() (in the tools repository). > (But as it turns out, the code I needed to fix was in buildbot, > not winetest.) Maybe the WineTestBot code needs a similar fix? In testbot/src/TestLauncher/TestLauncher.c? -- Francois Gouget <fgouget(a)codeweavers.com>

Sept. 4, 2012

Re: Building winetest-latest.exe?

by Francois Gouget

On Sun, 2 Sep 2012, Vincent Povirk wrote: > As I understand it, you need to set up a mingw build as in > http://wiki.winehq.org/CompilingDLLsUsingMingw The above page says to use mingw32 but this version of MinGW is pretty old (4.2) and, as far as I can tell, can only build 32bit Windows binaries. So I don't think that's what winetest / WineTestBot use. I think nowadays the packages to use is gcc-mingw-w64 & co (currently 4.6). See also this discussion: http://www.winehq.org/pipermail/wine-devel/2012-July/096183.html -- Francois Gouget <fgouget(a)free.fr> http://fgouget.free.fr/ The nice thing about meditation is that it makes doing nothing quite respectable -- Paul Dean

Sept. 4, 2012

Re: [1/2] gdiplus: Add some tests for scaling an image with various pixel offset modes.

by Vincent Povirk

> match = memcmp(dst_8x1, td[i].image, sizeof(dst_8x1)) == 0; > + if (!match) > + todo_wine ok(match, "%d: data should match\n", i); > + else > ok(match, "%d: data should match\n", i); This sort of thing makes me nervous. Couldn't you do this in such a way that we'll notice if the existing tests break, like adding a todo field to the structure?

Sept. 4, 2012

Re: d3dx9_36 [try 3]: Implement D3DXSHRotate

by Rico Schüller

On 04.09.2012 19:31, Nozomi Kodama wrote: > Your mail makes me thing two or three remarks. > > I agree that in the plain wine, code must be optimized to use less > memory as possible and to be the fastest as possible. > But, the tests are here to check that the implemented function behaves > like the windows one. > So, they don't need to be optimized. So, what is the point to worry > about efficiency? > > It is not important that I allocate 100 floats for my array even if I > use 49 only. > In the same idea, the time in the loop is not important. It is the same > if I spend 1s in a loop or 1/100s. They are there for testing. > Tests work, it is OK. I think function is optimized in plain wine, so I > can send the patch. Tests don't work. I must figure out why. While I agree with you, partly, one problem doesn't make much trouble, but if you add a couple of problems the app might explode. See the bad example (using a bigger index) for accessing the array... it may work for your machine, but the app may crash on another. You may overwrite some stuff and it is very hard to debug. For the floats, I think it shows that you are familiar with the code and know what you are doing. The question is, why don't you use 1000000 floats then, shouldn't be a problem at all? And you'll never know the next guy tries to implement something and copies your test code (that's not recommended at all) or just gets an idea and does the same and voila we got more and more of the hassle. Just think about it... I guess you took a look at test_D3DXSHRotateZ which is also a very bad example, but I missed it somehow when it was added. And still no one fixed it yet and that's how bad code and code style comes to that file... > > About coding style, I can not understand the policy. > If I look in the d3dx9_36/math.c file, I can see almost as much styles > as functions. > So, complaining about the style I use is a non-sense for me. Sure that's the problem. d3dx9_36 (especially math.c) is a very bad example when it comes to code style. Do we have to make it worse? Why do we need to produce more bad examples, if it is relatively easy to write clean code? Cheers Rico

Sept. 4, 2012

Re: d3dx9_36 [try 3]: Implement D3DXSHRotate

by Nozomi Kodama

Your mail makes me thing two or three remarks. I agree that in the plain wine, code must be optimized to use less memory as possible and to be the fastest as possible. But, the tests are here to check that the implemented function behaves like the windows one. So, they don't need to be optimized. So, what is the point to worry about efficiency? It is not important that I allocate 100 floats for my array even if I use 49 only. In the same idea, the time in the loop is not important. It is the same if I spend 1s in a loop or 1/100s. They are there for testing. Tests work, it is OK. I think function is optimized in plain wine, so I can send the patch. Tests don't work. I must figure out why. About coding style, I can not understand the policy. If I look in the d3dx9_36/math.c file, I can see almost as much styles as functions. So, complaining about the style I use is a non-sense for me. Nozomi ________________________________ De : Rico Schüller <kgbricola(a)web.de> À : wine-devel(a)winehq.org; Nozomi Kodama <nozomi.kodama(a)yahoo.com> Envoyé le : Mardi 4 septembre 2012 2h44 Objet : Re: d3dx9_36 [try 3]: Implement D3DXSHRotate On 04.09.2012 13:19, Nozomi Kodama wrote: + FLOAT expected, in[100], out[100], ... + for (i = 0; i < 49; i++) + in[i] = i + 1.01f; I guess this belongs together. I'd like to make some suggestions, please don't take it personally. When you got a comment on a patch, read it carefully. Think about it, if it makes sens to do the change (just adding what I've said is not always the best, it just gives you a hint, that you may improve some things, the list is not always complete and different people may have different suggestions) and try to improve your patch. After you are done, read your patch. Well mistakes happen, especially when you try to get something done really fast. (1) Take you some time and read it again and see if you've done that as best as possible and if you are happy with the code you've done, then submit it. If you've found a bug don't submit, fix it and go to #1. The earlier a bug is found the cheaper is it to solve it. I hope this helps a bit and it hopefully saves you some time to get a patch in the feature accepted. Thought I'd like to thank you for your patience and hopefully I haven't annoyed you and we'll see some good quality patches in the feature from you. :-) Cheers Rico

Sept. 4, 2012

Re: d3dx9_36 [try 3]: Implement D3DXSHRotate

by Nozomi Kodama

I agree with Rico. It's hard to find issues in your own code, which makes it all the more important to check it thoroughly. Still, at least try to be careful with the code style: +static void RotateX(FLOAT *out, UINT order, FLOAT a, FLOAT *in) I think it's better to only use lower case for the (private) function name. Something like "rotatex" or "rotate_x" or whatever. I can agree with the rotate name, but not with the X. X in capital is there to mean the X-axis. In math, one never uses a lower case letter for an axis. So, it would become hard to understand what means x. + if ( order < 2 ) + return; Please indent it correctly. OK. + return; +} Unneeded. One always said to me that it is a good pratice that a function returns something..... +FLOAT* WINAPI D3DXSHRotate(FLOAT *out, UINT order, CONST D3DXMATRIX *matrix, CONST FLOAT *in) Fix the FLOAT*. I don't understand. + return; +} Again. Also, the patch is full of trailing whitespaces, please fix those too. OK Nozomi

Sept. 4, 2012

Re: d3dx9_36 [try 3]: Implement D3DXSHRotate

by Matteo Bruni

2012/9/4 Rico Schüller <kgbricola(a)web.de>: > On 04.09.2012 13:19, Nozomi Kodama wrote: > + FLOAT expected, in[100], out[100], > ... > + for (i = 0; i < 49; i++) > + in[i] = i + 1.01f; > > I guess this belongs together. > > I'd like to make some suggestions, please don't take it personally. > > When you got a comment on a patch, read it carefully. Think about it, if it > makes sens to do the change (just adding what I've said is not always the > best, it just gives you a hint, that you may improve some things, the list > is not always complete and different people may have different suggestions) > and try to improve your patch. After you are done, read your patch. Well > mistakes happen, especially when you try to get something done really fast. > (1) Take you some time and read it again and see if you've done that as best > as possible and if you are happy with the code you've done, then submit it. > If you've found a bug don't submit, fix it and go to #1. > > The earlier a bug is found the cheaper is it to solve it. I hope this helps > a bit and it hopefully saves you some time to get a patch in the feature > accepted. Thought I'd like to thank you for your patience and hopefully I > haven't annoyed you and we'll see some good quality patches in the feature > from you. :-) > > Cheers > Rico > I agree with Rico. It's hard to find issues in your own code, which makes it all the more important to check it thoroughly. Still, at least try to be careful with the code style: +static void RotateX(FLOAT *out, UINT order, FLOAT a, FLOAT *in) I think it's better to only use lower case for the (private) function name. Something like "rotatex" or "rotate_x" or whatever. + if ( order < 2 ) + return; Please indent it correctly. + return; +} Unneeded. +FLOAT* WINAPI D3DXSHRotate(FLOAT *out, UINT order, CONST D3DXMATRIX *matrix, CONST FLOAT *in) Fix the FLOAT*. + return; +} Again. Also, the patch is full of trailing whitespaces, please fix those too.

Sept. 4, 2012

Re: wininet: Set last error for invalid URL argument

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 http://testbot.winehq.org/JobDetails.pl?Key=21380 Your paranoid android. === W2KPROSP4 (32 bit internet) === Timeout

Sept. 4, 2012

Re: [2/2] gdiplus: Add preliminary support for pixel offset modes.

by Vincent Povirk

> return sample_bitmap_pixel(src_rect, bits, width, height, > - gdip_round(point->X), gdip_round(point->Y), attributes); > + point->X + pixel_offset, point->Y + pixel_offset, attributes); > + } This is going to break tiling. By removing gdip_round you're implicitly truncating the number instead of flooring, which will give you the wrong result when X or Y is negative. I suggest explicitly adding a floorf.

Sept. 4, 2012