When DEBUG isn't defined, but PR_LOGGING is we end up trying to log a DebugOnly value. > 0:23.55 /Users/ericrahm/dev/mozilla-central/dom/plugins/base/nsNPAPIPluginInstance.cpp:570:99: error: cannot convert 'DebugOnly<NPError>' to 'NPError' (aka 'short') without a conversion operator > 0:23.55 window->clipRect.top, window->clipRect.bottom, window->clipRect.left, window->clipRect.right, (NPError)error)); > 0:23.55 ^~~~~~~~~~~~~~ > 0:23.55 /Users/ericrahm/dev/mozilla-central/dom/plugins/base/nsPluginLogging.h:79:40: note: expanded from macro 'NPP_PLUGIN_LOG' > 0:23.55 PR_LOG(nsPluginLogging::gNPPLog, a, b); \ > 0:23.55 ^ > 0:23.56 /Users/ericrahm/dev/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/include/nspr/prlog.h:181:19: note: expanded from macro 'PR_LOG' > 0:23.56 PR_LogPrint _args; \ > 0:23.56 ^ > 0:23.56 1 error generated.

Comment on attachment 8497036 [details] [diff] [review] Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds Don't use DebugOnly if the build has logging enabled.

Comment on attachment 8497036 [details] [diff] [review] Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds Review of attachment 8497036 [details] [diff] [review]: ----------------------------------------------------------------- John, bsmedberg indicated you could take a look at this small review.

Comment on attachment 8497036 [details] [diff] [review] Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds r=me

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8e58a613da

Backed out for Linux Werror bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/e0baa8287713 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2724707&repo=mozilla-inbound

(This probably needs to be conditional on PLUGIN_LOGGING, since the logging macro in question here only uses its arguments if PLUGIN_LOGGING is defined.)

(In reply to Daniel Holbert [:dholbert] from comment #7) > (This probably needs to be conditional on PLUGIN_LOGGING, since the logging > macro in question here only uses its arguments if PLUGIN_LOGGING is defined.) That fails too [1]. The issue appears to be related to including prlog.h, then including nsPluginLogging.h. nsPluginLogging.h does bad things like trying to force NSPR logging in a header, defining an NSPR internal that then overrides it's own ifdefs. This bug has been around for about 12 years at least [2]. Anyhow the rundown of the bustage is: #include "prlog.h" => defines it's include guards => FORCE_PR_LOG isn't set, so stubs out PR_LOG and friends => FORCE_PR_LOG isn't set, so PR_LOGGING doesn't get set #include "nsPluginLogging.h" => defines FORCE_PR_LOG => defines PR_LOGGING (which it most certainly should not) => makes sure PLUGIN_LOGGING is always defined => #includes "prlog.h", this does nothing b/c it hits its include guards => sets up some macros that just make you do a bunch of PR_LogFlushes b/c PR_LOG is stubbed out So then we get to my change, PR_LOGGING has been force defined, PLUGIN_LOGGING has been force defined, so I don't have a good test to see if the value is going to be used. [1] https://tbpl.mozilla.org/?tree=Try&rev=7d2790d31a62 [2] https://github.com/mozilla/gecko-dev/commit/973ae91db598f40050070a0300ff21ce06f582e3

John, this changed just enough that you might want to review again. Really the only change is that nsPluginLogging.h no longer #defines PR_LOGGING.

Won't this still cause problems in builds with PR_LOGGING defined, but with PLUGIN_LOGGING *not* defined? (It looks like PLUGIN_LOGGING is defined by default, so maybe that's not worth worrying about. Still, it looks like it's set up to let you enable/disable it as you like...) It might be simpler to just drop the DebugOnly<> version of this variable, and pass it to mozilla::unused, with a comment saying something like: // 'error' is only used if this is a logging-enabled build. // That is somewhat complex to check, so we just use "unused" // to suppress any compiler warnings in build configurations // where the logging is a no-op.

Okay I just did what Daniel recommended, we can leave the horribleness that is nsPluginLogging.h for another day.

Comment on attachment 8499221 [details] [diff] [review] Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds >Bug 1074415 - Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds. r=johns (The commit message should have r=dholbert now, I guess) >- DebugOnly<NPError> error; >+ NPError error; [...] > window->clipRect.top, window->clipRect.bottom, window->clipRect.left, window->clipRect.right, (NPError)error)); Drop the (NPError) cast in the actual logging invocation -- I don't think we need it, now that that's the actual type. r=me with that.

try: https://tbpl.mozilla.org/?tree=Try&rev=7462b0fb74ec remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8feab238d757