68036 - warnings in nsAutoLock.h when I build an optimized build

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[(not reading, please use [e-mail] instead)]

here comes a patch, chris or scott, can you review?

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: fix, please review.

Comment 2

[David :Bienvenu]

I guess the other choice is to #ifdef DEBUG the whole thing, e.g.,
#ifndef DEBUG
PR_ExitMonitor(mMonitor)
#else
PRStatus status = PR_ExitMonitor(...)
NS_ASSERTION(PR_SUCCEEDED(status)
#endif
I think I'll defer to whatever scc and waterson want to do for this. 

Comment 3

[Chris Waterson]

What is the warning, and wow does this fix it?

Comment 4

[(not reading, please use [e-mail] instead)]

sorry, it's an unused variable warning.

in optimized builds, NS_ASSERTION() is a no-op, so we never checked status.

Unused variable `PRStatus status'

Comment 5

[Chris Waterson]

But with your changes, isn't the variable *still* unused? Seems like doing 
something like bienvenu suggests, or even hackier:

  #ifdef DEBUG
  PRStatus status =
  #endif
    PR_EnterMonitor(mMonitor);

Would be necessary.

Comment 6

[Brendan Eich [:brendan]]

I like waterson's minimal-change #ifdef.

/be

Comment 7

[(not reading, please use [e-mail] instead)]

no, it isn't unused, because we declare it, and the use it by assigning to it.

but agree, I like waterson's trick better.  I'll submit a new patch.

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: new patch, ala waterson.

Comment 9

[Brendan Eich [:brendan]]

Seth, i'm going to be a style whiner: for short (fits in one page or screenful)
#ifdef...#endif blocks, the trailing comment after the #endif that restates or
summarizes the #if or #ifdef condition tends to be clutter; does it not get in
the way here, for you, when reading from the status declaration onward?

Also, compiler pedantry alert: a def (or set) is not a use.  A use of foo would
be a statement of the form bar = foo; or baz(foo); while a set of foo does not
use the value.  Therefore, assigning to status whether in the initializer or in
a separate statement sets a value that is not used.

/be

Comment 10

[(not reading, please use [e-mail] instead)]

some background:

I build with --enable-pedantic, I've found that if I fix all the warnings I find
I'm less likely to break the ports.

my original patch (declare, then assign) removed the 'unused variable' warning,
the patch seemed reasonable, the compiler was quiet, so I was happy.

new patch coming...

Comment 11

[Brendan Eich [:brendan]]

Hah!  A gcc --pedantic bug!  I don't care enough to report it.  I hope someone
does, just to get an uber-pedantic rationale for the bogus difference between an
initialized declaration and an uninitialized one followed by an assignment.

/be

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: new patch

Comment 13

[(not reading, please use [e-mail] instead)]

checked in to the mailnews perf branch, it will come over in the merge...

Comment 14

[Ben Bucksch (:BenB)]

Thanks for fixing the warning - it is very annoying, because it seems to come up
for every 3. file that is compiled optimzed, i.e. thousands of times.

Comment 15

[(not reading, please use [e-mail] instead)]

after we land, I'll bring this fix over by hand.

Comment 16

[(not reading, please use [e-mail] instead)]

fixed.

Comment 17

[Aleksey Nogin]

*** Bug 50083 has been marked as a duplicate of this bug. ***