On 11/15/07, Gerald Pfeifer gerald@pfeifer.com wrote:
On Thu, 15 Nov 2007, James Hawkins wrote:
ChangeLog: Fix use of signed versus unsigned variables.
This change is unnecessarily complicated.
How do you propose to address the following?
string.c: In function 'msi_addstring': string.c:208: warning: comparison between signed and unsigned string.c: In function 'msi_addstringW': string.c:260: warning: comparison between signed and unsigned string.c: In function 'msi_string2idW': string.c:400: warning: comparison between signed and unsigned
Turn off the warning.
Also, the only actual bug of this type that was in string.c (comparison..always false) has been fixed:
Happy to hear that!
All these patches to silence extraneous warnings makes me wonder whether you really understand the code or if you're just trying to get rid of warnings...
I'm mostly trying to get bugs fixed. In the last couple of weeks my patches have addressed several realy bugs and/or triggered others to look into bugs, both of which I'd claim is a good thing. ;-)
Ok, but this patch didn't fix any bug and it's makes an already complicated module that much more complicated.
Reducing the number of warnings is one way to catch regressions easier and get bugs addressed (not all warnings really are due to real bugs, of course, but that's hard to see up front).
I don't see how silencing warnings catches any regression. It's hard to see for you because you're changing code you don't understand.
And, no, I am not an expert of the entire Wine codebase, and certainly not to the level you and others are for those pieces you maintain. And the MSI code apparently is one of the more tricky ones...
We all appreciate your effort, but we could use your resources in more helpful ways. I suggest you look through bugzilla, pick out a bug, and try to fix that bug. You'll become more familiar with the code that way and you'll be fixing an actual reported bug. If you really want to keep sending in patches for these warnings, please be more conservative. Unsigned comparisons for less than 0 are actual bugs, so you can be sure about those, but the other warnings you need to spend a lot of time to see if there really is any bug. Warnings are just warnings, not errors, and there's a reason we don't turn all the warnings on.