Closed
Bug 1204403
Opened 9 years ago
Closed 9 years ago
Fix -Wshadow warnings in xpcom
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: cpeterson, Assigned: cpeterson)
References
Details
Attachments
(2 files)
2.73 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
60.19 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Part 1: Fix -Wshadow warnings in xpcom/io. These two warnings were real (but minor) bugs. When one variable's name shadowed another's, I renamed both variables to ensure that I did not miss a renaming and leave a name referencing the wrong variable. xpcom/io/nsMultiplexInputStream.cpp:102:16 [-Wshadow] declaration shadows a local variable xpcom/io/nsMultiplexInputStream.cpp:120:14 [-Wshadow] declaration shadows a local variable
Attachment #8660540 -
Flags: review?(continuation)
Assignee | ||
Comment 1•9 years ago
|
||
Part 2: Fix -Wshadow warnings that do not appear to be actual bugs. xpcom/base/nsStatusReporterManager.cpp:177:3 [-Wshadow] declaration shadows a local variable xpcom/base/nsStatusReporterManager.cpp:178:3 [-Wshadow] declaration shadows a local variable xpcom/base/nsStatusReporterManager.cpp:209:7 [-Wshadow] declaration shadows a local variable xpcom/base/nsStatusReporterManager.cpp:217:3 [-Wshadow] declaration shadows a local variable xpcom/ds/nsVariant.cpp:1019:29 [-Wshadow] declaration shadows a local variable xpcom/glue/GenericModule.cpp:74:19 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:611:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:645:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:667:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:698:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:732:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:755:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:790:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:820:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:844:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:886:27 [-Wshadow] declaration shadows a local variable xpcom/glue/tests/gtest/TestThreadUtils.cpp:916:27 [-Wshadow] declaration shadows a local variable xpcom/io/nsEscape.cpp:514:16 [-Wshadow] declaration shadows a local variable xpcom/io/nsLocalFileCommon.cpp:141:14 [-Wshadow] declaration shadows a local variable xpcom/io/nsMultiplexInputStream.cpp:102:16 [-Wshadow] declaration shadows a local variable xpcom/io/nsMultiplexInputStream.cpp:120:14 [-Wshadow] declaration shadows a local variable xpcom/tests/TestHashtables.cpp:643:19 [-Wshadow] declaration shadows a local variable xpcom/tests/TestRacingServiceManager.cpp:273:31 [-Wshadow] declaration shadows a local variable xpcom/tests/TestRacingServiceManager.cpp:295:31 [-Wshadow] declaration shadows a local variable xpcom/tests/nsIFileEnumerator.cpp:28:27 [-Wshadow] declaration shadows a local variable nsTArray-inl.h:270:11: warning: declaration shadows a variable in namespace '(anonymous)' [-Wshadow]
Attachment #8660541 -
Flags: review?(continuation)
Comment 2•9 years ago
|
||
Would you like me to review these patches, Nathan? (I'm not particularly attached to it, but I don't have anything in my review queue right now.)
Flags: needinfo?(nfroyd)
Comment 3•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > Would you like me to review these patches, Nathan? (I'm not particularly > attached to it, but I don't have anything in my review queue right now.) That'd be great, thank you!
Flags: needinfo?(nfroyd)
Comment 4•9 years ago
|
||
Comment on attachment 8660540 [details] [diff] [review] part-1-Wshadow_nsMultiplexInputStream.patch Review of attachment 8660540 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/io/nsMultiplexInputStream.cpp @@ +90,5 @@ > > static nsresult > AvailableMaybeSeek(nsIInputStream* aStream, uint64_t* aResult) > { > + nsresult rvAvailable = aStream->Available(aResult); Please leave |rvAvailable| as |rv| to match standard practice. @@ +110,5 @@ > > static nsresult > TellMaybeSeek(nsISeekableStream* aSeekable, int64_t* aResult) > { > + nsresult rvTell = aSeekable->Tell(aResult); Ditto for |rvTell|.
Attachment #8660540 -
Flags: review?(continuation) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8660541 [details] [diff] [review] part-2-Wshadow_xpcom.patch Review of attachment 8660541 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/moz.build @@ +153,5 @@ > if CONFIG['MOZ_WIDGET_GTK']: > CXXFLAGS += CONFIG['TK_CFLAGS'] > + > +if CONFIG['GNU_CXX']: > + CXXFLAGS += ['-Wshadow'] This is GCC only? Do we not have shadow for clang? ::: xpcom/base/nsStatusReporterManager.cpp @@ +77,5 @@ > #define DUMP(o, s) \ > do { \ > const char* s2 = (s); \ > uint32_t dummy; \ > + nsresult rvWrite = (o)->Write((s2), strlen(s2), &dummy); \ |rvDump| seems like it would cause less collisions with other code than |rvWrite|. ::: xpcom/glue/nsCRTGlue.cpp @@ +282,5 @@ > > void > NS_MakeRandomString(char* aBuf, int32_t aBufLen) > { > +#define TABLE_SIZE 36 Maybe use MOZ_ARRAY_LENGTH(table) instead of TABLE_SIZE or I can file a followup for that.
Attachment #8660541 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #5) > Comment on attachment 8660541 [details] [diff] [review] > part-2-Wshadow_xpcom.patch > > Review of attachment 8660541 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: xpcom/base/moz.build > @@ +153,5 @@ > > if CONFIG['MOZ_WIDGET_GTK']: > > CXXFLAGS += CONFIG['TK_CFLAGS'] > > + > > +if CONFIG['GNU_CXX']: > > + CXXFLAGS += ['-Wshadow'] > > This is GCC only? Do we not have shadow for clang? With this patch, xpcom/base has no -Wshadow warnings with GCC or clang. CONFIG['GNU_CXX'] includes both GCC and clang so -Wshadow will be enabled for both compilers.
https://hg.mozilla.org/mozilla-central/rev/0361ecab9e24
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 9•9 years ago
|
||
I had 45 minutes of head-scratching in bug 1203427 caused by a shadowed local variable. It was in xpcom/ and so this bug should have identified it immediately... but it was in Mac-only code and therefore didn't get caught by the new GCC-only -Wshadow checking. Is there any chance that -Wshadow can be turned on for clang as well?
Flags: needinfo?(cpeterson)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #9) > Is there any chance that -Wshadow can be turned on for clang as well? That is odd. AFAIU, the CONFIG['GNU_CXX'] in moz.build should include both gcc and clang (because clang pretends to be gcc). CONFIG['CLANG_CXX'] should only be needed to differentiate between clang and gcc. In which particular file/function was the shadowed local variable? I will double-check my assumptions about CONFIG['GNU_CXX'].
Flags: needinfo?(cpeterson)
Comment 11•9 years ago
|
||
> In which particular file/function was the shadowed local variable?
Oh, it was in xpcom/threads/nsTimerImpl.cpp, and I see now that xpcom/threads/moz.build doesn't have the -Wshadow addition. My mistake, sorry for the noise.
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #11) > Oh, it was in xpcom/threads/nsTimerImpl.cpp, and I see now that > xpcom/threads/moz.build doesn't have the -Wshadow addition. Thanks. I will double-check why I didn't add -Wshadow in that directory.
Comment 13•9 years ago
|
||
> Thanks. I will double-check why I didn't add -Wshadow in that directory.
I just tried it on Linux and there were no errors. (And then I deliberately added a shadowed variable just to double-check and I *did* get an error as expected.)
You need to log in
before you can comment on or make changes to this bug.
Description
•