April 2016 - Wine-Devel - list.winehq.org

Re: [PATCH] msvcr120: Add remainder (try3)

by Piotr Caban

On 04/06/16 10:16, Piotr Caban wrote: > Currently the tests are not working on wine (both i386 and x86_64). Please ignore that - I didn't run configure. Thanks, Piotr

April 6, 2016

Re: [PATCH] msvcr120: Add remainder (try3)

by Piotr Caban

Hi Daniel, On 04/06/16 01:00, Daniel Lehman wrote: > Try2: handle y==0 > Try3: add tests and error handling for both x and y, but I have a question: > > Based on the tests, remainder on 32-bit Windows differs from 64-bit Windows in errno handling. Other math functions, like acosf, behave the same - if tests for it were added, the current Wine implementation would differ from 32-bit Windows > > The #ifdef is admittedly ugly but makes it match both Windows 32-bit and 64-bit > > is it preferable to match Windows exactly? or could I drop the #ifdef and use the 64-bit errno handling, like acosf? could I skip the tests that differ on 32-bit? It depends on the function. In this case I don't think anything bad will happen if errno is set on invalid input even so it's not on Windows. I think it's OK to ignore the difference in this case and run some tests only in 32-bit or 64-bit environment. If you decide to be not fully compatible with native - please leave a comment in code. Currently the tests are not working on wine (both i386 and x86_64). Thanks, Piotr

April 6, 2016

Re: user32: Use top level window as dialog parent for modal dialogs.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > > The source of the regression that your patch is supposedly tries to fix is > > that the dialog code enables wrong window at the end of dialog message loop. > No, it fixes issue I found while looking at this, but it's a separated > and isolated issue. I don't touch the code that you ask me to write a > test for. You fail to understand that. I perfectly understand what you are trying to fix, and you actually mentioned it yourself already: "This is part of the fix for bug 40282". And that bug is a regression caused by your previous changes not backed up by the tests, and I already explained that the source of the regression is in wrong handling of enable/disable of the dialog owner, and once again I'd like to point out that fixing this without any tests simply can't work. > BTW, your tests don't test what you ask me to test and they were enough > for you to write a wrong fix. Otherwise you'd know that this code in the > end of the loop should be removed. Then again, it's not related to this > particular patch. Well, at lease I have written some tests before I tried to fix anything, and I'm not claiming that my fix is perfect/correct/useful at all. You are more than welcome to show your own exhaustive tests that check every detail of the dialog owner enable/disable logic. P.S. I'm not the person who commits patches to Wine, why are you arguing with me if it's pretty clear that you are not going to write any tests at all in order to demonstrate that your patch is correct? -- Dmitry.

April 6, 2016

Re: krnl386.exe16: Do not reassign default handles after they got closed.

by Sebastian Lackner

On 06.04.2016 05:50, Alexandre Julliard wrote: > Sebastian Lackner <sebastian(a)fds-team.de> writes: > >> @@ -363,7 +363,7 @@ HFILE16 WINAPI _lclose16( HFILE16 hFile ) >> } >> TRACE("%d (handle32=%p)\n", hFile, dos_handles[hFile] ); >> CloseHandle( dos_handles[hFile] ); >> - dos_handles[hFile] = 0; >> + dos_handles[hFile] = (hFile < 5) ? INVALID_HANDLE_VALUE : 0; > > This will cause functions that take default handles to not return the > correct errors. Is that really how it's supposed to work? What bug is > this for? > As far as I remember it basically solves two bugs at once: * First of all, apps interpret a returned HFILE of 0 from file APIs as failure. This can happen when apps call _lclose(0) before, or when FILE_InitProcessDosHandles does not properly initialize them. The second case, failure to initialize dos_handles[*] during init, will be fixed in a separate patch. * I also remember that Michael ran various tests to verify the return value of _lclose(*), and it showed that Windows returns 0 when trying to close such handle multiple times. I am aware that we still call CloseHandle() on INVALID_HANDLE_VALUE in Wine, but this is probably well-defined enough to avoid additional checks. @ Michael: Could you please double-check that my statements above are correct? If you still have your tests, please also attach them for further review. Regards, Sebastian

April 6, 2016

Re: user32: Use top level window as dialog parent for modal dialogs.

by Jacek Caban

On 4/6/16 5:32 AM, Dmitry Timoshkov wrote: > The source of the regression that your patch is supposedly tries to fix is > that the dialog code enables wrong window at the end of dialog message loop. No, it fixes issue I found while looking at this, but it's a separated and isolated issue. I don't touch the code that you ask me to write a test for. You fail to understand that. BTW, your tests don't test what you ask me to test and they were enough for you to write a wrong fix. Otherwise you'd know that this code in the end of the loop should be removed. Then again, it's not related to this particular patch. Jacek

April 6, 2016

[GSoC] Further work on the Visual C++ iostream library

by Piotr Caban

Hi Iván, could you please provide us with timeline for your project? It doesn't have to be very specific. Thanks, Piotr

April 6, 2016

Re: [tools] winemono.php: Set default version to 4.6.2.

by Vincent Povirk

I don't think so. On Tue, Apr 5, 2016 at 11:23 PM, Alexandre Julliard <julliard(a)winehq.org> wrote: > Vincent Povirk <madewokherd(a)gmail.com> writes: > >> @@ -4,7 +4,7 @@ >> */ >> >> // Default version if none given >> -$sVersion = '4.6.0'; >> +$sVersion = '4.6.2'; > > Do we need this? Is there anything that actually uses the default > version? > > -- > Alexandre Julliard > julliard(a)winehq.org

April 6, 2016

Re: [tools] winemono.php: Set default version to 4.6.2.

by Alexandre Julliard

Vincent Povirk <madewokherd(a)gmail.com> writes: > @@ -4,7 +4,7 @@ > */ > > // Default version if none given > -$sVersion = '4.6.0'; > +$sVersion = '4.6.2'; Do we need this? Is there anything that actually uses the default version? -- Alexandre Julliard julliard(a)winehq.org

April 6, 2016

Re: krnl386.exe16: Do not reassign default handles after they got closed.

by Alexandre Julliard

Sebastian Lackner <sebastian(a)fds-team.de> writes: > @@ -363,7 +363,7 @@ HFILE16 WINAPI _lclose16( HFILE16 hFile ) > } > TRACE("%d (handle32=%p)\n", hFile, dos_handles[hFile] ); > CloseHandle( dos_handles[hFile] ); > - dos_handles[hFile] = 0; > + dos_handles[hFile] = (hFile < 5) ? INVALID_HANDLE_VALUE : 0; This will cause functions that take default handles to not return the correct errors. Is that really how it's supposed to work? What bug is this for? -- Alexandre Julliard julliard(a)winehq.org

April 6, 2016

Re: user32: Use top level window as dialog parent for modal dialogs.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > >> I will ask you one more time to go back to the code and being technical > >> instead of personal. > > Hmm? My comments have pure technical background, all I ask is about > > adding more tests to show that your patch is correct and to prevent > > possible regressions in future. > > And if you want me to take you seriously as a reviewer, then start > reading the code. Let me read it for you. We call EnableWindow() in two > functions: > > 1) DIALOG_CreateIndirect > Before and after my patch, we use exactly the same window in > DIALOG_CreateIndirect: I didn't change that logic at all. Do you see it? > > 2) DIALOG_DoDialogBox > In DIALOG_DoDialogBox we currently use GetWindow(GW_OWNER) (which is a > subject for future changes). My patch doesn't change it as well. Here we > have two cases: > > 2a) dialog is WS_CHILD: For WS_CHILD, owner is NULL with and without my > patch. > 2b) For other cases, we have a real owner, but then again, the owner > will be the same before and after my patch. We know that, I added a test > and fix for that in [1] and I ensure that in tests included to the patch. > > End of story. > > As I told you, I will write a patch for that, when I will work on that. > This patch is about other, unrelated, issue that I do test in the patch. The source of the regression that your patch is supposedly tries to fix is that the dialog code enables wrong window at the end of dialog message loop. Neither old nor current tests don't even pretend that they are testing that correct window is being enabled/disabled. And that's what I ask for: add such tests. You had refused to add more tests previously and as the result we have a regression. Regarding the claims that only WS_CHILD style makes a difference: apply my tests and in the tests replace the checks (WS_CHILD|WS_POPUP) == WS_CHILD by just testing for WS_CHILD and the test will fail. If your tests don't fail then your tests simply don't check all the possible style combinations that may lead to failures. > > If that's too much of an additional effort for > > you for some reason then I have no idea why you decided to work on this > > at all, just drop this, user32 is too complex area for hasty craft, and > > requires a lot of time to write the tests and investigate things. > > And that's personal again. Then every comment requesting to add more tests that actually confirm that the proposed changes are correct while the sender doesn't want to write such tests are personal. Refusing to create more tests to prove the correctness of the patch doesn't add a lot of confidence to that patch at all. Writing more tests is always an additional work, and claiming that a request to add the tests is personal sounds at least strange. -- Dmitry.

April 6, 2016