Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Fix nearly all warnings building image/ on Linux, debug, with clang

RESOLVED FIXED in mozilla12



6 years ago
6 years ago


(Reporter: Waldo, Assigned: Waldo)



Firefox Tracking Flags

(Not tracked)



(1 attachment)

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.
Attachment #585243 - Flags: review?(bobbyholley+bmo)
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 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! :-)
Attachment #585243 - Flags: review?(bobbyholley+bmo)
Attachment #585243 - Flags: review+
Attachment #585243 - Flags: feedback?(joe)
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.
Attachment #585243 - Flags: feedback?(joe) → feedback+
Target Milestone: --- → mozilla12
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.