Fix -Wshadow warnings in xpcom

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Created attachment 8660540 [details] [diff] [review]
part-1-Wshadow_nsMultiplexInputStream.patch

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

3 years ago
Created attachment 8660541 [details] [diff] [review]
part-2-Wshadow_xpcom.patch

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)
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)
(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 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 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

3 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
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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

3 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)
> 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

3 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.
> 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.)
(Assignee)

Updated

3 years ago
Depends on: 1207031
(Assignee)

Updated

3 years ago
Blocks: 1272513
You need to log in before you can comment on or make changes to this bug.