Closed Bug 1204400 Opened 4 years ago Closed 4 years ago

Fix -Wshadow warnings in gfx/thebes and suppress those from Skia headers

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Fix -Wshadow warnings in gfx/thebes and suppress those from Skia headers.

gfx/thebes/gfxCoreTextShaper.cpp:524:41 [-Wshadow] declaration shadows a local variable
gfx/thebes/gfxCoreTextShaper.cpp:534:45 [-Wshadow] declaration shadows a local variable
gfx/thebes/gfxMacPlatformFontList.mm:175:39 [-Wshadow] declaration shadows a local variable
gfx/thebes/gfxMacPlatformFontList.mm:868:21 [-Wshadow] declaration shadows a local variable
gfx/thebes/gfxMacPlatformFontList.mm:873:31 [-Wshadow] declaration shadows a local variable
gfx/thebes/gfxScriptItemizer.cpp:67:55 [-Wshadow] declaration shadows a field of 'gfxScriptItemizer'
gfx/thebes/gfxScriptItemizer.cpp:100:34 [-Wshadow] declaration shadows a field of 'gfxScriptItemizer'
gfx/thebes/gfxTextRun.cpp:1893:18 [-Wshadow] declaration shadows a local variable

GLContext.h:1540:60: error: declaration of 'ref' shadows a member of 'this' [-Werror=shadow]
GLContext.h:1546:90: error: declaration of 'ref' shadows a member of 'this' [-Werror=shadow]

gfx/thebes/gfxFontconfigFonts.cpp:1035:41: error: declaration of 'entry' shadows a previous local [-Werror=shadow]
gfx/thebes/gfxFT2FontList.cpp:937:19: error: declaration of 'faceList' shadows a previous local [-Werror=shadow-compatible-local]

../../../../workspace/gecko/gfx/skia/skia/include/core/SkPoint.h:30:28: error: declaration of 'y' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/core/SkPoint.h:30:28: error: declaration of 'x' shadows a member of 'this' [-Werror=shadow]
...
../../../../workspace/gecko/gfx/skia/skia/include/core/SkTArray.h:92:41: error: declaration of 'count' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/core/SkTArray.h:144:43: error: declaration of 'count' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/core/SkTArray.h:373:75: error: declaration of 'count' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/core/SkTArray.h:378:67: error: declaration of 'count' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/gpu/GrRenderTarget.h:151:9: error: declaration of 'desc' shadows a member of 'this' [-Werror=shadow]
../../../../workspace/gecko/gfx/skia/skia/include/gpu/GrRenderTarget.h:151:9: error: declaration of 'isWrapped' shadows a member of 'this' [-Werror=shadow]
Attachment #8660534 - Flags: review?(bgirard)
Comment on attachment 8660534 [details] [diff] [review]
Wshadow_thebes.patch

Review of attachment 8660534 [details] [diff] [review]:
-----------------------------------------------------------------

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.

::: gfx/thebes/gfxCoreTextShaper.cpp
@@ -298,5 @@
>      double runWidth = ::CTRunGetTypographicBounds(aCTRun, ::CFRangeMake(0, 0),
>                                                    nullptr, nullptr, nullptr);
>  
>      nsAutoTArray<gfxShapedText::DetailedGlyph,1> detailedGlyphs;
> -    gfxShapedText::CompressedGlyph g;

This g was unused because the g variables below shadowed it.

::: gfx/thebes/gfxFcPlatformFontList.cpp
@@ +369,5 @@
>      }
>  
>      nsRefPtr<gfxCharacterMap> charmap;
>      nsresult rv;
> +    bool symbolFont = false; // currently ignored

Reuse this symbolFont variable and move "currently ignored" comment up here.

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +1107,5 @@
>                  uint32_t cmapLen = CFDataGetLength(cmapTable);
>                  nsRefPtr<gfxCharacterMap> charmap = new gfxCharacterMap();
>                  uint32_t offset;
> +                bool unicodeFont = false; // ignored
> +                bool symbolFont = false;

This symbolFont actually *is* used, so remove the "ignored" comment. Also move the definitions of unicodeFont and symbolFont down so all this block of definitions matches the function parameter order when they are passed to ReadCMAP() below.
Comment on attachment 8660534 [details] [diff] [review]
Wshadow_thebes.patch

Review of attachment 8660534 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +844,5 @@
>                                                     kCFAllocatorNull);
>          if (!str) {
>              return nullptr;
>          }
> +        index = 2;

len -> index is a non trivial change IMO, best keep these warning fix to have no impact. Why not use 'length' or something else similar if that's also used?
Attachment #8660534 - Flags: review?(bgirard) → review+
(In reply to Benoit Girard (:BenWa) from comment #2)
> len -> index is a non trivial change IMO, best keep these warning fix to
> have no impact. Why not use 'length' or something else similar if that's
> also used?

Thanks. I will use 'length' as you suggested.
https://hg.mozilla.org/mozilla-central/rev/3d0b47f7a37b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.