Re: dlls/msi/string.c -- fix use of signed versus unsigned (RESEND)

[James Hawkins]

On 11/14/07, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
> Last one for today, promised. ;-)
>
> Gerald
>
> ---------- Forwarded message ----------
> From: Gerald Pfeifer <gerald(a)pfeifer.com>
> To:  <wine-patches(a)winehq.org>
> Date: Sat, 3 Nov 2007 20:02:18 +0100 (CET)
> Subject: dlls/msi/string.c -- fix use of signed versus unsigned
>
> This was a bit more tricky, but I hope I got it right.  Basically the
> problem here was that we used a parameter/variable n of type UINT both
> as a true unsigned (the return of the hash function) and as a true signed
> variable (comparing against -1).
>
> This removes the following three compiler diagnostics that I've been
> seeing.  In other words, this is also addressing very real issues!
>
> string.c: In function 'msi_addstring':
>   string.c:208: warning: comparison of unsigned expression < 0 is always false
>   string.c: In function 'msi_addstringW':
>   string.c:260: warning: comparison of unsigned expression < 0 is always false
>   string.c: In function 'msi_string2idW':
>   string.c:400: warning: comparison between signed and unsigned
>
> Gerald
>
> ChangeLog:
> Fix use of signed versus unsigned variables.

This change is unnecessarily complicated.  Also, the only actual bug
of this type that was in string.c (comparison..always false) has been
fixed:

commit 1c1cf26997cdf491642b2086ad817a79ced99a2e
Author: James Hawkins <truiken(a)gmail.com>
Date:   Mon Nov 5 04:49:07 2007 -0500

    msi: Explicitly check the returned value against -1 as the
variable is unsigned.

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...

-- 
James Hawkins

dlls/msi/string.c -- fix use of signed versus unsigned (RESEND)

[Gerald Pfeifer]

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
> 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. ;-)

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).

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...

Gerald

dlls/msi/string.c -- fix use of signed versus unsigned (RESEND)

[James Hawkins]

On 11/15/07, Gerald Pfeifer <gerald(a)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.

-- 
James Hawkins

dlls/msi/string.c -- fix use of signed versus unsigned (RESEND)

[Gerald Pfeifer]

On Thu, 15 Nov 2007, James Hawkins wrote:
> I don't see how silencing warnings catches any regression.

With default options older versions of GCC currently issue two warnings 
for all of Wine, so any warning regression immediately jumps out for 
analysis to see whether it is a real regression.  If instead of two the 
base is 2000, just another one is easy to miss.  That's the experience
we made with GCC, and some other projects I have worked on.
> 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

I actually plan to focus on those.  There are about fifty left.  Just
for patches I have sent already or on my disk it would be nice to see
those considered because I did invest time on those.

Gerald