In bug 75621 we turned off the -Wshadow build warning with gcc. The primary reason we turned it off was warning noise in the JS engine -- warning noise that's no longer present now that JS is build as C++ rather than C. So we should turn -Wshadow back on, since it frequently warns about real bugs.

Well, except nanojit has tons of constructors that cause warnings; not sure if we consider them worth fixing or want to keep this turned off instead. (I have patches for a few other things in my tree.)

http://groups.google.com/group/mozilla.dev.platform/msg/9353917079cfec22 suggests nnethercote is ok with fixing this in nanojit. I guess, then, the question is what convention we want to use for the classes that have members and parameters to the constructor (or occasionally other methods) that have the same name. That's the common pattern in JS and especially nanojit: see JSAutoTransferRequest constructor, JSObjectMap constructor, PropertyCacheEntry::assign, a bunch of methods on JSString, and a very large number of classes in nanojit, such as most of the constructors in Container.h and LIR.h. The convention we tend to use in other parts of Gecko is that the members are mFoo and the parameters are aFoo (for "argument"). (Some people aren't exactly crazy about this convention, though.) Any idea what convention should be used here? For what it's worth, I started to write the patches to do this: http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6360f53e59be/enable-wshadow-js-warning http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/6360f53e59be/enable-wshadow but then decided it was too much work. (The patches I have are sufficient to avoid warnings in content/layout code from the JS headers, though.)

(In reply to comment #2) > > The convention we tend to use in other parts of Gecko is that the members are > mFoo and the parameters are aFoo (for "argument"). (Some people aren't exactly > crazy about this convention, though.) > > Any idea what convention should be used here? Nanojit is shared between Mozilla and Adobe and it doesn't follow the usual Mozilla conventions (indeed, it has very little in the way of formal conventions at all). Renaming class members seems like a very painful way to do this, I'd much rather only rename constructor arguments. A strawman suggestion, albeit one I'd be happy with: add '0' to the end? eg. Foo::Foo(int x0, int y0) : x(x0), y(y0) { ... }

I think i've seen similar warnings from sunpro compiler on nanojit and various tamarin classes. imho that particular warning doesn't pay for itself, but its moot, no point debating it. opinion? I'm not keen on aFoo as a convention for arguments, but I could be convinced to use mFoo or m_foo for nanojit class members, and i'm fine with Nick's more surgical convention as well. At some point, nanojit needs to get with the program and adopt a real (existing) convention; i'm partial to webkit's, actually, and parts of google's, and parts of mozilla's. (but not so partial to hybrid conventions, boo). Having just finished a big naming refactoring arc (the great opcode renaming) i'd just as soon let things sit in nanojit for awhile before doing another big rename. small renames are no problem at all.

Here's a partial patch. It fully deals with Nanojit headers. The rest of SpiderMonkey/TraceMonkey requires many more changes than Nanojit, I've done some of them. I used the '0' suffix convention almost everywhere, even for local variables that happen to shadow members, which is much nastier than the constructor case mentioned in comment 3. I only got partway because it was ickier than I'd hoped. And all those local variables shadowing members started making me nervous, enough for me to start understanding the merit of "mFoo"-style members.

Is this bug still relevant with tracejit gone?

Yes. It may be a little easier now.

This would have saved me a lot of time. :(

See bug 800659 comment 8. We should proceed with this without the JS engine opting into it.

We do need to make the JS *public* API headers not trigger -Wshadow warnings. Right now they trigger a bunch (though they keep changing), which are the bulk of the -Wshadow warnings I see in my build. I think to fix that, we probably need to compile a dummy file, including some set of public JS headers, somewhere in the js/src build, with -Wshadow and -Werror, so that JS hackers don't introduce new -Wshadow warnings.

Oh, and I probably shouldn't leave the misleading assigned-to field here. But I'd love if somebody else picked this up. (Oh, and the reason for wanting comment 10: otherwise it'll be pretty mean to JS hackers, since they'll trigger tinderbox red due to WARNINGS_AS_ERRORS in layout/style or elsewhere.)

https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/tip/enable-wshadow-js-warning may be a useful starting point for the warning fixes in the JS engine

I swear I assigned this to myself yesterday. There are tons of JS -Wshadow warnings, but most of them are dups. I have a patch that fixes them all and it's not that big. It's small enough that I'm not too worried about comment 11. I'm now working on patches for all the directories that have FAIL_ON_WARNINGS set. There are quite a few.

Interesting data point: this is easier to do for clang than GCC. In the following example: class X { int foo(); int bar(int foo); }; GCC warns about the |foo| parameter to bar() shadowing foo(), but clang doesn't. clang's behaviour is nicer, because that kind of shadowing isn't remotely a problem in practice. But GCC is the lowest common denominator, alas. And this does require quite a few more changes to placate GCC.

I've done a lot of the work to get this working. I don't think it's worth it. It was a huge PITA, because we had a lot of these warnings. I mostly just fixed the ones that were in FAIL_ON_WARNINGS directories; there were several hundred. That still leaves behind a few hundred with clang, and more with GCC. I don't think I fixed any actual bugs. There were plenty of "code smells", i.e. code that was a bit dicey looking, but not obviously incorrect. A lot of these were in long functions. If the compilers only warned about local parameters/variables that shadow other local variables, then I might recommend we turn this on. But the number of hoops we have to jump through to get various libraries (SpiderMonkey, ipc/chromium/, etc) to pass muster is awful, with little payback. --- Anyway, I'm attaching an omnibus patch that's enough to get it building on clang. The most common case is something like this: int foo = 3; ... if (...) { ... int foo = 4; ... } It might look silly in this form but it happens *all the time*. The most notable thing is that it happens a lot more in long functions. We have lots of long functions, especially in layout/ :( There are several ways to fix it. - Just reuse the first variable, if possible. This is very common for |rv| and similar short-term error return codes. - Put the first one in its own block, e.g.: { int foo = 3; ... } This is probably the best approach because it minimizes how much code sees the name. In long functions, this is actually a big deal. However, often the overlapping lifetimes of variable make it hard to do. - Try to rename one or both of them nicely. E.g. if the two conflicting vars have different types, preferably rename both of them. - If either has a single use, just inline the value at that use. - Finally, if none of these work, you have to rename one of them badly, e.g. the second one |foo2|. Really, the best solution a lot of the time is to *split your damn gigantic function into smaller pieces*. But I didn't want to do that to tons of unfamiliar code. (This is why I like using blocks to fix cases, because it's the next best thing.) Feedback is welcome.

Here's a patch -- that applies on top of the previous -- and goes partway to making it compile on GCC. (This is with --enable-warnings-as-errors, BTW.) The case in comment 14 is not the only one where GCC warns and clang doesn't; there are others, though I haven't worked out exactly where the dividing line lies. Something I forgot to mention: these patches are difficult to read, because often the variable being shadowed is dozens (or even hundreds) of lines away, so you don't see it in the diff. You really have to look at the full source code to understand it.

(In reply to Nicholas Nethercote [:njn] from comment #15) > It was a huge PITA, because we had a lot of these warnings. I mostly just > fixed the ones that were in FAIL_ON_WARNINGS directories; there were several > hundred. That still leaves behind a few hundred with clang, and more with > GCC. This is one of the reasons I dislike FAIL_ON_WARNINGS.

Hmm, thanks a lot Nick for delving through this. At this point, I think I agree that it's not worth anybody's time to get this fully working. Something that I've wanted for a long time, though, is enabling the build system to tell you which warnings you have introduced compared to the previous build. I think that goes a long way to help using compiler warnings locally when writing code. Also, Nick, perhaps it's worth filing a clang bug to ask for a variant of -Wshadow which only warns when masking local variables...

> This is one of the reasons I dislike FAIL_ON_WARNINGS. That's beside the point in this case. Imagine FAIL_ON_WARNINGS didn't exist. Even after fixing the .h file warnings that get repeated many times, there were still several hundred -Wshadow warnings (with GCC it may have been over 1000). And modifying the code to avoid those was non-trivial in a sizeable fraction of those cases. A good warning flag is one that (a) identifies defects reasonably often, and (b) is easy to work around in the false positive cases. -Wshadow scores badly on both counts, unfortunately.

(In reply to Nicholas Nethercote [:njn] from comment #19) > > This is one of the reasons I dislike FAIL_ON_WARNINGS. > > That's beside the point in this case. Imagine FAIL_ON_WARNINGS didn't > exist. Even after fixing the .h file warnings that get repeated many times, > there were still several hundred -Wshadow warnings (with GCC it may have > been over 1000). And modifying the code to avoid those was non-trivial in a > sizeable fraction of those cases. > > A good warning flag is one that (a) identifies defects reasonably often, and Multiple people have stories of spending significant amounts of time debugging things that -Wshadow would have told them about. Those things aren't in the tree anymore, though. So I'm not sure what data you're using here. > (b) is easy to work around in the false positive cases. I think it is easy to work around when you're the one introducing the warning. It's hard to do for code that somebody else wrote and you don't understand, but that's true for many other warnings as well. (When I look at the -Wshadow things in layout/ that scroll by in my builds (since I have it on, and have, I think, since we turned it off), I've definitely seen a bunch of them point to suspicious code that looks like it might not be doing what its author intended.)

I guess I'll file a separate bug for turning on -Wshadow within layout/, then.

I've been bitten by the lack of -Wshadow myself on a couple of occasions, so I understand its benefits. I *wanted* to turn it on, which is why I spent a whole day trying to do it. My conclusion was that the cost:benefit ratio was too high, at least for turning it on globally. Turning it on piecemeal is a far more viable way forward. But if you don't intend to fix the warnings, I don't see much point in it.

I still think fixing -Wshadow warnings in the JS public API headers (and keeping them out) is the first step to doing that, since they're the predominant source of warnings.

Since this directory has FAIL_ON_WARNINGS set, this will cause any shadowing warnings triggered by headers included in layout/style to cause errors. Once this patch is reviewed, I'll write and attach patches to fix the warnings needed to land this.

A better name here would be welcome.

Comment on attachment 714720 [details] [diff] [review] Fix -Wshadow warnings in public SPS headers. Review of attachment 714720 [details] [diff] [review]: ----------------------------------------------------------------- benoits.next()

Comment on attachment 714719 [details] [diff] [review] Fix -Wshadow warnings in layout/style. r=me

Comment on attachment 714721 [details] [diff] [review] Fix -Wshadow warnings in gfx that affect layout/style. Review of attachment 714721 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxQuaternion.h @@ +38,5 @@ > } > > gfxFloat theta = acos(dot); > gfxFloat rsintheta = 1/sqrt(1 - dot*dot); > + gfxFloat w_ = sin(aCoeff*theta)*rsintheta; rightWeight would work fine as a slightly more verbose name. The original quaternion slerp paper did not give a name to this quantity.

Comment on attachment 714718 [details] [diff] [review] Fix -Wshadow warnings in public JS API headers (those used from layout/style/). Review of attachment 714718 [details] [diff] [review]: ----------------------------------------------------------------- I'd prefer you to add a trailing '_' to the relevant data members, like I did in attachment 702706 [details] [diff] [review] in bug 800659. If there are any remaining clashes between arguments and methods, please use the "aFoo" form for the argument. Thanks.

Comment on attachment 714998 [details] [diff] [review] Fix -Wshadow warnings in public JS API headers (those used from layout/style/). Review of attachment 714998 [details] [diff] [review]: ----------------------------------------------------------------- Mostly good, you just missed a few, noted below. Thanks for making this change. SpiderMonkey's naming conventions don't really fit with the use of -Wshadow, and after quite some thought and experimentation this was the least worse way of doing it that I came up with. BTW, have you found any shadowing bugs in layout/? ::: js/public/CharacterEncoding.h @@ +89,5 @@ > typedef mozilla::Range<jschar> Base; > > public: > TwoByteChars() : Base() {} > + TwoByteChars(jschar *charsarg, size_t aLength) : Base(charsarg, aLength) {} s/charsarg/aChars/ @@ +90,5 @@ > > public: > TwoByteChars() : Base() {} > + TwoByteChars(jschar *charsarg, size_t aLength) : Base(charsarg, aLength) {} > + TwoByteChars(const jschar *charsarg, size_t aLength) : Base(const_cast<jschar *>(charsarg), aLength) {} Ditto. @@ +105,5 @@ > > public: > StableTwoByteChars() : Base() {} > + StableTwoByteChars(jschar *charsarg, size_t aLength) : Base(charsarg, aLength) {} > + StableTwoByteChars(const jschar *charsarg, size_t aLength) : Base(const_cast<jschar *>(charsarg), aLength) {} Ditto, twice. ::: js/public/Vector.h @@ +378,5 @@ > > class Range { > friend class Vector; > T *cur, *end; > + Range(T *curarg, T *endarg) : cur(curarg), end(endarg) {} Change the parameters back to |cur| and |end|, and rename the data members as |cur_| and |end_|. @@ +446,5 @@ > } > void infallibleAppendN(const T &t, size_t n) { > internalAppendN(t, n); > } > + template <class U> void infallibleAppend(const U *aBegin, const U *endarg) { s/endarg/aEnd/

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/761d4d55680d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/43fa20226909 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b8d67b35d82 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fa2028f2e3c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ff75b06e2330 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/df03beb83b39 (The html parser patch isn't needed for layout/style, but I definitely hit it somewhere.)

https://hg.mozilla.org/mozilla-central/rev/761d4d55680d https://hg.mozilla.org/mozilla-central/rev/43fa20226909 https://hg.mozilla.org/mozilla-central/rev/2b8d67b35d82 https://hg.mozilla.org/mozilla-central/rev/4fa2028f2e3c https://hg.mozilla.org/mozilla-central/rev/ff75b06e2330 https://hg.mozilla.org/mozilla-central/rev/df03beb83b39

FWIW, GCC 4.8 has changed its behaviour to be more like clang's (i.e. better). From http://gcc.gnu.org/gcc-4.8/changes.html: "The option -Wshadow no longer warns if a declaration shadows a function declaration, unless the former declares a function or pointer to function, because this is a common and valid case in real-world code."

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18) > At this point, I think I agree that it's not worth anybody's time to get > this fully working. Something that I've wanted for a long time, though, is > enabling the build system to tell you which warnings you have introduced > compared to the previous build. I think that goes a long way to help using > compiler warnings locally when writing code. Can someone introduce this to feature to TryBuilder for FF and TB? (I have seen tons of warnings from latest headers and am a little bit perplexed at why it was not noticed by the person who modified them last time.) Even a crude form of grepping and a very crude summary/filtering by awk/perl would go a long way to eliminate the NEW INTRODUCTION of warnings. TIA

(In reply to ISHIKAWA, chiaki from comment #42) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #18) > > > At this point, I think I agree that it's not worth anybody's time to get > > this fully working. Something that I've wanted for a long time, though, is > > enabling the build system to tell you which warnings you have introduced > > compared to the previous build. I think that goes a long way to help using > > compiler warnings locally when writing code. > > Can someone introduce this to feature to TryBuilder for FF and TB? I submitted a bugzilla entry Bug 858543 - Creating "Warning Line Police" to reduce compile-time warning to discuss this. I think it is important for a long time health of the source base. TIA

Rather than leaving this open indefinitely after a bunch of patches landed for it >1 year ago, perhaps we should change the scope to just cover layout/style (the parts that landed, mostly) and track any remaining work (like fixing this in other /layout subdirs) in a followup? (It looks like Ms2ger tried to reduce the scope & closed the bug when merging to m-c in comment 38, but dbaron reverted that a few days later when landing the last patch. dbaron, do you want to still leave this open?)

Maybe I should revisit this now that C-C TB uses similar infrastructure as M-C (i.e., mozharness.) Previously |mach| began to work with C-C TB for about a year (?), but a test suite called mozmill for TB did not work with |mach|. But maybe it would not be long before |mach| can invoke mozmill. TIA