Closed
Bug 75621
Opened 24 years ago
Closed 23 years ago
Wshadow flag is too noisy
Categories
(SeaMonkey :: Build Config, defect, P3)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.3
People
(Reporter: pete, Assigned: pete)
Details
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
The Wshadow flag is to noisy and makes it hard to debug.
Is it really needed?
--pete
Assignee | ||
Comment 1•24 years ago
|
||
Comment 2•24 years ago
|
||
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
Assignee | ||
Comment 3•24 years ago
|
||
-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
Comment 4•24 years ago
|
||
Chris & Chris - any input on removing Wshadow?
Priority: -- → P3
Target Milestone: --- → mozilla0.9.2
Comment 6•24 years ago
|
||
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
Assignee | ||
Comment 7•24 years ago
|
||
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
Assignee | ||
Comment 8•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Assignee | ||
Comment 10•24 years ago
|
||
Who can i bug to sr this patch?
Thanks
--pete
Assignee | ||
Comment 11•24 years ago
|
||
Comment 12•24 years ago
|
||
Actually, I'd rather you _didn't_ remove this warning. Shadowing is a frequent
source of bugs.
Assignee | ||
Comment 13•24 years ago
|
||
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
Comment 14•24 years ago
|
||
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
Comment 15•24 years ago
|
||
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).
Comment 16•24 years ago
|
||
pete, i think cls gave you what you wanted (sr), so fire away when you get a
shot at an open tree.
Assignee | ||
Comment 17•24 years ago
|
||
Checked in. Changing resolution to fixed.
Thanks
--pete
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 18•24 years ago
|
||
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?
Comment 19•24 years ago
|
||
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
Comment 20•24 years ago
|
||
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 → ---
Comment 21•24 years ago
|
||
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.
Comment 22•24 years ago
|
||
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
Comment 23•24 years ago
|
||
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.?
Assignee | ||
Comment 24•24 years ago
|
||
> 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
Comment 25•24 years ago
|
||
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.)
Comment 26•24 years ago
|
||
s/with the warnings/with the warnings through the preprocessor/
Comment 27•24 years ago
|
||
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...
Comment 28•24 years ago
|
||
dbaron: Why can't we use those options? It's not as if any Mozilla code *wants*
the old index (now strchr) function.
/be
Comment 29•24 years ago
|
||
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.
Assignee | ||
Comment 30•24 years ago
|
||
Assignee | ||
Comment 31•24 years ago
|
||
So we can:
export WSHADOW=TRUE
when we want it on.
When we want it off we can just:
unset WSHADOW
--pete
Comment 32•24 years ago
|
||
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.
Comment 33•24 years ago
|
||
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.
Comment 34•24 years ago
|
||
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.
Comment 35•24 years ago
|
||
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?
Assignee | ||
Comment 36•23 years ago
|
||
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
Assignee | ||
Comment 37•23 years ago
|
||
cahnging email address
Assignee: petejc → petejc
Status: REOPENED → NEW
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 38•23 years ago
|
||
please reopen this bug if you have a problem.
--pete
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
Comment 40•22 years ago
|
||
Bug 197583 is a great example of why we shouldn't have done this.
Updated•20 years ago
|
Product: Browser → Seamonkey
Comment 41•20 years ago
|
||
This caused bug 276719. I'd like to undo this.
Updated•20 years ago
|
Summary: Wshadow flag is to noisy → Wshadow flag is too noisy
Comment 42•15 years ago
|
||
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.
Description
•