Just tried to build gfx/2d with FAIL_ON_WARNINGS, but I hit these build warnings on windows: { DrawTargetSkia.cpp e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\skia\GrGlyph.h(42) : error C2220: warning treated as error - no 'object' file generated e:\builds\moz2_slave\try-w32-d-00000000000000000000\build\obj-firefox\dist\include\skia\GrGlyph.h(42) : warning C4003: not enough actual parameters for macro 'free' 2 } https://tbpl.mozilla.org/php/getParsedLog.php?id=28861021&tree=Try { PathD2D.cpp e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(439) : error C2220: warning treated as error - no 'object' file generated e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(439) : warning C4003: not enough actual parameters for macro 'free' e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTypes.h(505) : warning C4003: not enough actual parameters for macro 'free' e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTemplates.h(96) : warning C4003: not enough actual parameters for macro 'free' e:\builds\moz2_slave\try-w32-0000000000000000000000\build\obj-firefox\dist\include\skia\SkTemplates.h(122) : warning C4003: not enough actual parameters for macro 'free' } https://tbpl.mozilla.org/php/getParsedLog.php?id=28861029&tree=Try#error0

(It looks like it's confusing the "free()" function defined in these methods with a macro to implement the normal malloc/free functionality. There's probably a "free_XYZ(void* buf)" function in play, which some header has a "#defines free" pointing to, which messes up any other usage of "free" as a method name.)

(sorry, meant to say 'looks like it's confusing the "free()" method defined in these classes')

like maybe here: > 18 #define free moz_free http://mxr.mozilla.org/mozilla-central/source/gfx/graphite2/src/MozGrMalloc.h?rev=7c3cd4824f94#18 or something similar.

OK, the macro definition that's causing trouble is here: > 19 #define free(_) moz_free(_) http://mxr.mozilla.org/mozilla-central/source/memory/mozalloc/mozalloc_macro_wrappers.h#12 (We end up including that via BaseRect.h's #include of nsDebug.h, which #includes nscore.h, which #includes mozalloc_macro_wrappers.h )

So, I see two possible solutions: (1) Avoid exposing the "#define free" macro to SkTypes.h and GrGlyph.h (e.g. by #including them before anything else, or by adding an #undef) ...or... (2) Rename the "free()" methods on SkTypes.h and GrGlyph.h to something else (e.g. freePtr() and freeGlyph()), and rename all of their callers in Skia code. I lean towards option 1, I think, because it seems bogus that we're randomly evaluating Skia headers with a different "free" definition, depending on whether nsDebug.h (and friends) are included, and in what order.

Actually, probably the best thing to do would be to remove "#include nsDebug.h" from BaseRect.h. I think that might be what's causing this. (That include was added in bug 914919, so that we could downgrade some MOZ_ASSERT assertions to be non-fatal NS_ASSERTION (the only nonfatal assertion we have at the moment). Ms2ger then pointed out in bug 914919 comment 15 that we technically shouldn't have NS_* junk in gfx/2d, and roc points out in the next comment that this is (currently) the only way to get nonfatal assertions, but we should use MOZ_NONFATAL_ASSERT if/when it becomes available.)

(Even with BaseRect fixed, we hit another #include-nsDebug-before-skia issue in convolver.cpp and image_operations.cpp, both of which include nsAlgorithm.h, which unnecessarily includes nsDebug.h. I filed bug 925066 on fixing that.)

I've normally been #undefing free in this situation elsewhere, and it's been making me sadface :( Any better solution would be most welcome!

yeah... "#undef free" scares me, because it seems like that could make us use unmatched malloc/free implementations and potentially do something bad, in the unlikely event that we actually bother to call malloc/free.

(In reply to Daniel Holbert [:dholbert] from comment #7) > (Even with BaseRect fixed, we hit another #include-nsDebug-before-skia issue > in convolver.cpp and image_operations.cpp Ah, so these *also* get the "free" macro definition via their includes of basictypes.h, because that header ends with: > 252 #include "nscore.h" // pick up mozalloc operator new() etc. http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/basictypes.h

OK, I've got a 3-patch patch stack (with trivial patches, just reordering #includes) to fix comment 7.

So as noted in comment 10, we're getting the "#define free" via basictypes.h. If we include SkTypes before that, we're fine. This makes that change for convolver.h, which fixes the convolver.cpp issues.

This makes image_operations.cpp include its own header first (which gets it a SkTypes.h include), before it includes basictypes.h and gets the problematic #define. (This makes us also align with our general strategy of making .cpp files include their own .h file first.)

image_operations.cpp also hits this issue for "SkTemplates.h" which gets pulled in via skia/SkBitmap.h. So, this patch makes us include SkBitmap a bit earlier (before basictypes.h), which keeps it from getting thrown by the #define free.

Actually, bug 926275 might be a better way to address this.

Comment on attachment 816274 [details] [diff] [review] part 1: make convolver.h include SkTypes before it sees the #define free Canceling review requests, since IIUC bug 926275 is really the right fix for this.

This was FIXED by bug 926275.