Closed Bug 75621 Opened 24 years ago Closed 23 years ago

Wshadow flag is too noisy

Categories

(SeaMonkey :: Build Config, defect, P3)

x86
FreeBSD
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.3

People

(Reporter: pete, Assigned: pete)

Details

Attachments

(5 files)

The Wshadow flag is to noisy and makes it hard to debug. Is it really needed? --pete
Attached patch removing Wshadow compiler flag (deleted) — Splinter Review
are we sure these are warnings that nobody cares about at all?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Wshadow flag is to noisy → Wshadow flag is to noisy
-Wshadow Warn whenever a local variable shadows another local variable. On various searches i've done it seems there are simular complaints of Wshadow in g++ compiler. http://gcc.gnu.org/ml/gcc/2000-03/msg00011.html Pulling it might not be a bad idea. --pete
Chris & Chris - any input on removing Wshadow?
Priority: -- → P3
Target Milestone: --- → mozilla0.9.2
r=cls
r=leaf, as well. I'll reassign this to you, pete, since you're probably going to be checking in the fix =)
Assignee: leaf → petejc
Leaf i still need an sr right? i guess this fix will go in when the tree reopens as i see the tree is burning fiercely right now . . . ;-) --pete
Attached patch updated patch (deleted) — Splinter Review
Attached patch Less tabs Doh! (deleted) — Splinter Review
Who can i bug to sr this patch? Thanks --pete
Actually, I'd rather you _didn't_ remove this warning. Shadowing is a frequent source of bugs.
Ok Chris, as long as it does more good that noise. I've been looking at the same Wshadow warnings for like two years now . . . It makes debuging harder. I have no problem doing this locally. If there is a consensus here i can mark this bug invalid. Thanks --pete
It'd be nice to get rid of the ancient pre-POSIX Unix 'index' pollution, or if not that, then of its warning. Any ideas? /be
Pete, sr=cls. If we're not going to get rid of the -Wshadow flag, then someone (drivers? managers?) needs to hound people to clean up their code. The extra warnings really do make build logs hard to debug. If you want to see the extreme case, try compiling the qt port sometime. Not only do they use a local variable named 'index' in their exported headers but they also manage to annoy the compiler into thinking that they are shadowing member variables when it's not so clear that they are. From /usr/lib/qt-2.2.4/include/qevent.h : class Q_EXPORT QMouseEvent : public QEvent { public: QMouseEvent( Type type, const QPoint &pos, int button, int state ); QMouseEvent( Type type, const QPoint &pos, const QPoint&globalPos, int button, int state ) : QEvent(type), p(pos), g(globalPos), b(button),s((ushort)state) {}; ... } With -Wshadow, gcc complains about type, pos, globalPos, button & state as shadowing a member of this. Afaict, none of those variables are members of the QMouseEvent or its parent classes (QEvent & Qt).
pete, i think cls gave you what you wanted (sr), so fire away when you get a shot at an open tree.
Checked in. Changing resolution to fixed. Thanks --pete
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
So we no longer warn about subtle failures to propagate return values like: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/imap/src/nsImapMailFolder.cpp&mark=2020#2010 Is this really a good thing?
cls: we are *not* going to tell people to use indx instead of index, just because of a bad old global function name from Unix pre-history. Let's push on selecting warning suppression or filtering, instead. /be
Wait, wasn't there enough controversy and question-asking to not check this in with one sr=? Can we please discuss the trade-off and alternatives more? I agree with dbaron that bad rv shadowing should not be warning-free. /be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Well, I have to be the devil's advocate, but isn't that the purpose of sr=? You only need one? I thought the tradeoffs were fairly clear. -Wshadow warns about more than just the single case that dbaron mentioned. In order to keep -Wshadow and *not* remain in the same losing situation that we were in, someone would need to clean-up the cases that trigger the bogus shadow warnings as well as the legitimate ones (defined as dbaron's case). You said that we aren't going make people stop using 'index' because of some ancient Unix-ism so what's the alternative that I'm missing here? Waiting for someone to fix gcc? Doing post processing on compiler output in the build system (no thanks). I really don't think that people that trigger dbaron's case are going to be enlightened by this one legitimate shadow warning hidden in the storm of bogus ones.
No, sr= doesn't mean debate is preempted, or objections ignored. Let's discuss. Why not filter output as slamm's warning stuff for tinderbox does, at least until we can get gcc improvements _a la_ the old SGI/MIPS compilers feature of warning-suppression-by-number? No need to convince me that too many warnings == no warnings, but too few can be worse than a few more vital ones. /be
When this was checked in it reduced the warning count on tinderbox by about 125. Most of those look like things we should be warning about. (There are a few that seem questionable in the GIF and JPEG decoders.) Where are all the cases where we shouldn't be warning? If they're all in the QT port, couldn't we remove -Wshadow when compiling mozilla/widget/src/qt/, mozilla/gfx/src/qt/, etc.?
> Where are all the cases where we shouldn't be warning? Everywhere! What inspired this bug is the warnings i've been looking at in the JS module for the last couple of years. I created a patch and posted to the js newsgroup changing various warnings like `index' to `indx' etc. But i was vehemintly opposed. Which i understand, because index is not wrong. Another case was some dom work i was doing w/ jst and my mistakes were hidden in a noise of shadow warnings. So in that case i changed the variables in question which made reviewing the patch harder for jst though. What if we make Wshadow an environment variable? WSHADOW Then we can easily set and unset it . . . --pete
What are the warnings in js/src/jsapi.h about anyway? I ran one of the files with the warnings and the first occurence of the string "index" was one that it warned about. And I see them with gcc 3.0 but not gcc 2.96 or gcc 2.7.2.3. So is this just a bug with gcc 3.0? (Shrike tinderbox before the patch, which uses gcc 2.91.66, doesn't show the ones in JS either.)
s/with the warnings/with the warnings through the preprocessor/
Oh, I see, it's the global declaration of a builtin... one that goes away if we specify -ansi (or -std=c99, or -fno-builtin). But I guess we probably couldn't get away with any of those for other reasons...
dbaron: Why can't we use those options? It's not as if any Mozilla code *wants* the old index (now strchr) function. /be
Hmmm. Perhaps we can use -ansi. -fno-builtins seems to disable optimization of certain builtins too, so I don't think we'd want that.
Attached patch Wshadow as an env variable (deleted) — Splinter Review
So we can: export WSHADOW=TRUE when we want it on. When we want it off we can just: unset WSHADOW --pete
And it turns out -ansi only works (for hiding the builtins) for C files (not C++) and also causes "linux" not to be #define-d.
Ok, we're sitting in the "discussion" limbo again. Qt is *not* the only place we're seeing this problem. It is by far the worst as the condition that triggers the bogus warnings is in the Qt headers as well as the mozilla source. I'm really not interested in adding a hack to the build system to clean up these bogus warnings. I would much prefer the builds to have pristine output. If you are relying upon the tinderbox to show you the bogus cases, umm, why? They've been filtered, remember? Add -Wshadow back to your CFLAGS and look at any of the "shadows" warnings in gfx, widget, layout, mailnews, etc.
I wasn't relying on tinderbox. I see them on my own machine with gcc 3.0 but not with gcc 2.96 or 2.7.2.3.
What about adding an autoconf test (using gcc -Wshadow -Werror) so that we define -Wshadow for any compiler where it doesn't give the bogus warnings, and then work on getting the gcc people to have an option so that we can get rid of them?
I am moving target to 0.9.3 but i would really like to resolve this closed. --pete
Target Milestone: mozilla0.9.2 → mozilla0.9.3
cahnging email address
Assignee: petejc → petejc
Status: REOPENED → NEW
Status: NEW → ASSIGNED
please reopen this bug if you have a problem. --pete
Status: ASSIGNED → RESOLVED
Closed: 24 years ago23 years ago
Resolution: --- → FIXED
verified.
Status: RESOLVED → VERIFIED
Bug 197583 is a great example of why we shouldn't have done this.
Product: Browser → Seamonkey
This caused bug 276719. I'd like to undo this.
Summary: Wshadow flag is to noisy → Wshadow flag is too noisy
I filed bug 563195 to turn -Wshadow back on. The main reason for turning it off is no longer an issue: the index warnings don't happen anymore now that JS is built as C++ rather than C.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: