1204403 - Fix -Wshadow warnings in xpcom

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: 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

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: 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]

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Andrew McCreight [:mccr8]]

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|.

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Chris Peterson [:cpeterson]]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0361ecab9e24

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0361ecab9e24

Comment 9

[Nicholas Nethercote [inactive]]

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?

Comment 10

[Chris Peterson [:cpeterson]]

(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'].

Comment 11

[Nicholas Nethercote [inactive]]

> 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.

Comment 12

[Chris Peterson [:cpeterson]]

(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

[Nicholas Nethercote [inactive]]

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