Last Comment Bug 714572 - Fix nearly all warnings building image/ on Linux, debug, with clang
: Fix nearly all warnings building image/ on Linux, debug, with clang
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All Linux
-- minor (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Milan Sreckovic [:milan]
Depends on:
  Show dependency treegraph
Reported: 2012-01-01 19:13 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-04 04:52 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (12.78 KB, patch)
2012-01-01 19:13 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bobbyholley: review+
joe: feedback+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-01 19:13:00 PST
Created attachment 585243 [details] [diff] [review]

After some lazy Sunday-afternoon warning-fixing culminating in this patch, there remains only one warning building image/, I think, due to this code:

void // static
nsPNGEncoder::ErrorCallback(png_structp png_ptr,
                            png_const_charp error_msg)
#ifdef DEBUG
	// XXX: these messages are probably useful callers...
        // use nsIConsoleService?
	PR_fprintf(PR_STDERR, "PNG Encoder: %s\n", error_msg);;
#if PNG_LIBPNG_VER < 10500
        longjmp(png_ptr->jmpbuf, 1);
        png_longjmp(png_ptr, 1);

With the in-tree png 1.4.8 we use png_ptr->jmpbuf, which png.h marks as a deprecated field.  I assume the next libpng update will eliminate this, and/or I don't have the domain-specific knowledge to write a fix.

Notes on the patch:

* clang won't warn deleting instances of classes with virtual methods but a non-virtual destructor if the class is marked final
* nsCOMPtr is null-initialized by default, so it seemed simplest to just remove the initialization of such fields
* in the interests of cleanliness I centralized mozilla/* includes at the start of all files I touched
* I'm not certain that all the nsIconChannel classes need MOZ_FINAL, but 1) they're all final in practice so it doesn't hurt, and 2) it seems best to be consistent

Try is churning away on the patch now.
Comment 1 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-01 22:00:37 PST
Everything builds fine on try.  I did skim the logs to see if errors remained.  gcc seems to spew some warnings that clang didn't, so there's more cleaning needed here, but for clang's purposes, this is good enough (modulo the png_ptr->jmpbuf bit).
Comment 2 User image Bobby Holley (:bholley) (busy with Stylo) 2012-01-02 11:29:56 PST
Comment on attachment 585243 [details] [diff] [review]

>+#if defined(MOZ_ENABLE_GNOMEUI) || defined(MOZ_ENABLE_GIO)

I don't really know what this is, but I'll trust you on it.

>-    class ExpirationTrackerObserver : public nsIObserver {
>+    class ExpirationTrackerObserver MOZ_FINAL : public nsIObserver {

This seems to be the main idea of the patch. It's fine with me, but I want to make sure joe's ok with putting these things everywhere. Flagging him for feedback.

r+ otherwise. Thanks for doing this! :-)
Comment 3 User image Joe Drew (not getting mail) 2012-01-03 10:45:56 PST
Comment on attachment 585243 [details] [diff] [review]

Review of attachment 585243 [details] [diff] [review]:

In the case of nsExpirationTracker and nsSupportsWeakReference, it sure would be nice if the base classes actually had virtual destructors, but since they don't, I'm OK with this solution.
Comment 4 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-03 20:32:19 PST
Comment 5 User image Marco Bonardo [::mak] 2012-01-04 04:52:15 PST

Note You need to log in before you can comment on or make changes to this bug.