Re: Question from new developer

[Bruno Jesus]

On Fri, Sep 21, 2012 at 8:41 PM, Chris Teague chris.tea...@gmail.com wrote:
 I submitted bug 31753 for an application that I use, and started
 making an attempt to fix it.  I started with the last few messages
 from the log:

 fixme:win:LockWindowUpdate (0x501c4), partial stub!
 fixme:win:LockWindowUpdate ((nil)), partial stub!
 wine: Unhandled page fault on read access to 0x0023 at address
 0x78680087 (thread 0009), starting debugger...

 Looking at LockWindowUpdate(), I discovered that it had no conformance
 tests.  I wrote a few covering all the possible scenarios I could
 think of (only 4 of them), and ran them on wine as well as cross
 compiling them and running on XP.  To my surprise, one of the tests
 fails on wine, but passes on WinXP!  This was great news, so I dug a
 little further.  Specifically the problem was attempting to do a
 unlock operation by passing NULL, when the the function was already
 unlocked.  Windows treats this as an error and returns 0, while wine
 returns 1.
 So I fixed it, patch is attached (also at:
 https://dl.dropbox.com/u/477050/0001-Fixed-LockWindowUpdate.txt)
 I have two big questions:
 1. I discovered later that I don't think this is the root of my
 original bug - and in fact doesn't seem to affect behavior of my
 program.  Is it still worth submitting?  I do believe it alleviates
 the need for the fixme in the LockWindowUpdate function.

The fixme is there because the function is far away from implemented,
you can read more about it at the long standing bug 52 (
http://bugs.winehq.org/show_bug.cgi?id=52 ).
IMO adding tests that prove something is wrong are always good.

 2. If it is worth submitting, could someone take a look at it and
 point out all the dumb things I've done?  I'm happy with the logic of
 the code, but I'm sure I've done something horribly wrong in terms of
 doing it the preferred way.

You should join all tests in the same function and use an existing
file. You need to fix the whitespace issues, you didn't follow the
surrounding code style (possibly your editor was configured
differently from the file). You are also not freeing the created
windows.

 Thanks,
 Chris Teague

I'm not used to reviewing patches, I hope someone else can do a better review.

Best wishes,
Bruno

re: Question from new developer

[Dan Kegel]

Chris wrote:
 1. I discovered later that I don't think this is the root of my
 original bug - and in fact doesn't seem to affect behavior of my
 program.  Is it still worth submitting?

Generally, fixes are more likely to get committed when they
don't break anything (did you run all the conformance tests?),
their tests cases pass on all versions of windows (did you send
your patch to testbot?), improve the behavior of a real program
(I don't think you have demonstrated this yet), and
are written from a position of relatively deep understanding.

So you might want to find a more solid fix for your first submission.

Welcome, and good luck!
- Dan