Closed
Bug 563195
Opened 15 years ago
Closed 2 years ago
Turn -Wshadow build option back on for layout/style
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla21
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [leave open])
Attachments
(9 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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.)
Assignee | ||
Comment 2•14 years ago
|
||
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.)
Comment 3•14 years ago
|
||
(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) { ... }
Comment 4•14 years ago
|
||
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.
Comment 5•14 years ago
|
||
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.
Comment 6•13 years ago
|
||
Is this bug still relevant with tracejit gone?
Assignee | ||
Comment 7•13 years ago
|
||
Yes. It may be a little easier now.
Updated•12 years ago
|
Attachment #456141 -
Attachment is obsolete: true
Comment 8•12 years ago
|
||
This would have saved me a lot of time. :(
Comment 9•12 years ago
|
||
See bug 800659 comment 8. We should proceed with this without the JS engine opting into it.
No longer depends on: 800659
Assignee | ||
Comment 10•12 years ago
|
||
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.
Assignee | ||
Comment 11•12 years ago
|
||
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.)
Assignee: dbaron → nobody
Assignee | ||
Comment 12•12 years ago
|
||
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
Comment 13•12 years ago
|
||
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.
Assignee: nobody → n.nethercote
Comment 14•12 years ago
|
||
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.
Comment 15•12 years ago
|
||
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.
Comment 16•12 years ago
|
||
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.
Assignee | ||
Comment 17•12 years ago
|
||
(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.
Comment 18•12 years ago
|
||
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...
Comment 19•12 years ago
|
||
> 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.
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 20•12 years ago
|
||
(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.)
Assignee | ||
Comment 21•12 years ago
|
||
I guess I'll file a separate bug for turning on -Wshadow within layout/, then.
Comment 22•12 years ago
|
||
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.
Assignee | ||
Comment 23•12 years ago
|
||
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.
Assignee | ||
Comment 24•12 years ago
|
||
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.
Attachment #714488 -
Flags: review?(ted)
Assignee | ||
Updated•12 years ago
|
Assignee: n.nethercote → dbaron
Updated•12 years ago
|
Attachment #714488 -
Flags: review?(ted) → review+
Assignee | ||
Comment 25•12 years ago
|
||
Attachment #714716 -
Flags: review?(hsivonen)
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #714717 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 27•12 years ago
|
||
Attachment #714718 -
Flags: review?(n.nethercote)
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #714719 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 29•12 years ago
|
||
Attachment #714720 -
Flags: review?(bjacob)
Assignee | ||
Comment 30•12 years ago
|
||
A better name here would be welcome.
Attachment #714721 -
Flags: review?(tterribe)
Comment 31•12 years ago
|
||
Comment on attachment 714720 [details] [diff] [review]
Fix -Wshadow warnings in public SPS headers.
Review of attachment 714720 [details] [diff] [review]:
-----------------------------------------------------------------
benoits.next()
Attachment #714720 -
Flags: review?(bjacob) → review?(bgirard)
Comment 32•12 years ago
|
||
Comment on attachment 714719 [details] [diff] [review]
Fix -Wshadow warnings in layout/style.
r=me
Attachment #714719 -
Flags: review?(bzbarsky) → review+
Updated•12 years ago
|
Attachment #714720 -
Flags: review?(bgirard) → review+
Comment 33•12 years ago
|
||
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.
Attachment #714721 -
Flags: review?(tterribe) → review+
Updated•12 years ago
|
Attachment #714717 -
Flags: review?(jwalden+bmo) → review+
Comment 34•12 years ago
|
||
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.
Attachment #714718 -
Flags: review?(n.nethercote) → review-
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #714998 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•12 years ago
|
Attachment #714718 -
Attachment is obsolete: true
Comment 36•12 years ago
|
||
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/
Attachment #714998 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 37•12 years ago
|
||
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.)
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Turn -Wshadow build option back on → Turn -Wshadow build option back on for layout/style
Comment 38•12 years ago
|
||
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
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Turn -Wshadow build option back on for layout/style → Turn -Wshadow build option back on for layout
Attachment #714716 -
Flags: review?(hsivonen) → review+
Assignee | ||
Comment 39•12 years ago
|
||
Whiteboard: [leave open]
Comment 40•12 years ago
|
||
Comment 41•12 years ago
|
||
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."
Comment 42•12 years ago
|
||
(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
Comment 43•12 years ago
|
||
(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
Comment 44•11 years ago
|
||
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?)
Flags: needinfo?(dbaron)
Comment 45•10 years ago
|
||
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
Updated•7 years ago
|
Product: Core → Firefox Build System
Assignee | ||
Updated•4 years ago
|
Assignee: dbaron → nobody
Status: REOPENED → NEW
Comment 46•2 years ago
|
||
Closing; file new bugs for followups.
Status: NEW → RESOLVED
Closed: 12 years ago → 2 years ago
Flags: needinfo?(dbaron)
Resolution: --- → FIXED
Updated•2 years ago
|
Assignee: nobody → dbaron
Assignee | ||
Updated•2 years ago
|
Summary: Turn -Wshadow build option back on for layout → Turn -Wshadow build option back on for layout/style
You need to log in
before you can comment on or make changes to this bug.
Description
•