Closed Bug 666664 Opened 8 years ago Closed 5 years ago

gfx/thebes - compiler warnings on mac

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: joey, Assigned: stevoooo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(3 files, 1 obsolete file)

% uname -a
Darwin banshee.local 10.7.4 Darwin Kernel Version 10.7.4: Mon Apr 18 21:24:17 PDT 2011; root:xnu-1504.14.12~3/RELEASE_X86_64 x86_64

/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::signature'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::flavor'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::length'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::numTables'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::reserved'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::totalSfntSize'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::majorVersion'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::minorVersion'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::metaOffset'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::metaCompLen'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::metaOrigLen'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::privOffset'
/mozilla/sandbox/gml/gfx/thebes/gfxUserFontSet.cpp:409: warning: 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader' declared with greater visibility than the type of its field 'CopyWOFFMetadata(const PRUint8*, PRUint32, nsTArray<unsigned char, nsTArrayDefaultAllocator>*, PRUint32*)::WOFFHeader::privLen'
/mozilla/sandbox/gml/gfx/thebes/GLContext.cpp:1285: warning: comparison between signed and unsigned integer expressions
/mozilla/sandbox/gml/gfx/thebes/gfxMacFont.cpp:65: warning: 'CGFontCreateWithPlatformFont' is deprecated (declared at /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/CoreGraphics.framework/Headers/CGFont.h:60)
/mozilla/sandbox/gml/gfx/thebes/gfxMacFont.cpp:65: warning: 'CGFontCreateWithPlatformFont' is deprecated (declared at /Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/CoreGraphics.framework/Headers/CGFont.h:60)
/mozilla/sandbox/gml/gfx/thebes/nsCoreAnimationSupport.mm:452: warning: comparison between signed and unsigned integer expressions
/mozilla/sandbox/gml/gfx/thebes/nsCoreAnimationSupport.mm:453: warning: comparison between signed and unsigned integer expressions
/mozilla/sandbox/gml/gfx/thebes/nsCoreAnimationSupport.mm:732: warning: comparison between signed and unsigned integer expressions
/mozilla/sandbox/gml/gfx/thebes/nsCoreAnimationSupport.mm:734: warning: comparison between signed and unsigned integer expressions
Whiteboard: [build_warnings]
Whiteboard: [build_warnings] → [build_warning]
Compile warning are a great starting bug. If you're interested in taking on a warning or two please reply here.

You can find information on contributing here:
https://wiki.mozilla.org/Platform/GFX/Contribute
Whiteboard: [build_warning] → [good first bug][mentor=bgirard@mozilla.com][build_warning]
Hi, my time is limited but I'd happy to take a look at some warnings.  Anywhere in particular that's a good place to start?
Fixing any warnings is great. Here is an up to date build log that will show recent warnings:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9464971&tree=Firefox&full=1

I would suggest fixing the warnings for a single file or so per patch, that way if the review for some fixes is tricky then we can land the patch for other files that are not.

Let me know if you have any questions.
Comment on attachment 598941 [details] [diff] [review]
Patch to fix warnings in gfx/thebes/gfxContext.cpp

Patch looks good. Keep attaching patch as you have time and let me know when you're done, I'll land them together.
Attachment #598941 - Flags: review+
Don't like having to use another variable, but couldn't seem to shift the warning just using a cast.
Gnnr, forgot to fix remaining warning in gfxFont.cpp - will update later.
Attachment #598977 - Attachment is obsolete: true
Attachment #599059 - Attachment is patch: true
Comment on attachment 599059 [details] [diff] [review]
Updated patch to fix all warnings in gfx/thebes/gfxFont.cpp

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

::: gfx/thebes/gfxFont.cpp
@@ +2116,2 @@
>          if (!breakHere) {
> +            if (ch32 >= 0x100) {

I assume there was a "comparison is always false" warning here, when compiling the 8-bit instance of the template where ch is a PRUint8? The expectation is that in this case, the compiler will know the comparison cannot succeed, because of the variable type, and so it can optimize away the whole if-statement.

My concern here is that introducing a new, wider variable means that the desired optimization here (and similarly below) is no longer quite as obvious - to detect it, the compiler will have to be tracking the _possible_ value that ch32 could have, based on its initialization from ch, rather than being able to infer it directly from the types in the expression.

So if we're going to make a change here, we should check that it doesn't affect the final optimized code on any compiler that we care about....
Yes, it was a "comparison always false" warning.  I wasn't overly happy about it either - perhaps a better way would be to just comment it as an optimisation and find some way of suppressing that specific instance of the warning.
Makes CopyWOFFMetadata a static private member of the gfxUserFontSet class.  Fixes several "... declared with greater visibility than the type of its field" warnings.
Attachment #599307 - Attachment is patch: true
Attachment #599307 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6317057db545
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdf3748265e

Are you still working on these Stephen? I'll be happy to review and land any additional patch.
Whiteboard: [good first bug][mentor=bgirard@mozilla.com][build_warning] → keep-open [good first bug][mentor=bgirard@mozilla.com][build_warning]
There's still some warnings to be fixed if someone is interested in working on some.
Assignee: stevoooo → nobody
Going to close out this bug. We can file a new bug for any remaining build warnings.
Assignee: nobody → stevoooo
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: keep-open [good first bug][mentor=bgirard@mozilla.com][build_warning] → [build_warning]
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.