835715 - modules/libjar/nsJARChannel.cpp:46:0: warning: "LOG" redefined

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Daniel Holbert [:dholbert]]

Right now we spam this warning:
{
 1:53.16 Warning: enabled by default in /scratch/work/builds/mozilla-inbound/mozilla/modules/libjar/nsJARChannel.cpp: "LOG" redefined
 1:53.16 /scratch/work/builds/mozilla-inbound/mozilla/modules/libjar/nsJARChannel.cpp:46:0: warning: "LOG" redefined [enabled by default]
 1:53.16 In file included from /scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/command_line.h:27:0,
 1:53.16                  from /scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/process_util.h:28,
 1:53.16                  from ../../dist/include/ipc/IPCMessageUtils.h:10,
 1:53.16                  from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowser.h:14,
 1:53.16                  from ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PBrowserChild.h:9,
 1:53.16                  from ../../dist/include/mozilla/dom/TabChild.h:11,
 1:53.16                  from ../../dist/include/mozilla/net/RemoteOpenFileChild.h:10,
 1:53.16                  from /scratch/work/builds/mozilla-inbound/mozilla/modules/libjar/nsJARChannel.cpp:24:
 1:53.16 /scratch/work/builds/mozilla-inbound/mozilla/ipc/chromium/src/base/logging.h:95:0: note: this is the location of the previous definition
}

We can just #undef LOG at the top of the cpp file (like we do in many other places to work around this).

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Masatoshi Kimura [:emk]]

Comment on attachment 707481 [details] [diff] [review]
fix v1

>+#ifdef LOG
>+#undef LOG
>+#endif

Are there any compilers that warn about unconditional #undef? If not, #ifdef...#endif is redundant.

Comment 3

[Daniel Holbert [:dholbert]]

I don't know.

I've seen both patterns in our code, and it feels cleaner to check whether it's defined before undefining it, so that's why I did it that way.  Doesn't really matter, though.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 707481 [details] [diff] [review]
fix v1

Per bug 835912 comment 2, it sounds like taras should review changes to this file; hence, kicking the review over to him.

taras, let me know if you have a preference on the "#ifdef LOG" wrapper, per prev. two comments; I'm fine either way.

Comment 5

[Jason Duell]

See bug 545995.  But IIRC since nsJARChannel isn't setting PR_FORCE_LOGGING, there should be no bad consequences from undefining chromium's LOG macro.  If we ever try to add FORCE_LOGGING it won't work until/unless we either do the same kludge as in 545995, or (better yet) we grab the bull by the horns and do a mass rename of s/LOG/.../ at some point in the tree.

Comment 6

[Daniel Holbert [:dholbert]]

I think this bug might be less hairy than that other bug.

IIUC (from a quick skim), that other bug is about receiving two different LOG defines from two different headers; in contrast, this bug is about a header-provided LOG clashing with a local LOG.

Given that we've got our local LOG right there, we don't need to wring our hands too much about undefining LOG.

We could also (trivially) do something like s/LOG/JARLOG/ in this file, but that feels like we'd just be giving up ownership of the word "LOG", which feels defeatist. :)

Comment 7

[Jason Duell]

> we don't need to wring our hands too much about undefining LOG.

Sounds good to me.  Maybe put a comment next to the #undef:

  // if you ever want to define PR_FORCE_LOGGING in this file, see bug 545995

Comment 8

[Daniel Holbert [:dholbert]]

Cool -- per IRC conversation, I think that comment is more related to the "#define LOG PR_LOG(...)" than to the #undef (since we're basically saying "warning: PR_LOG might not do what you expect").

So -- I'll add that comment, w/ that tweak.

Comment 9

[Daniel Holbert [:dholbert]]

(where "that tweak" = "putting it adjacent to the #define rather than the #undef")

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 707481 [details] [diff] [review]
fix v1

jduell granted r+ over IRC, so I pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76dea17faf44

Comment 11

[(dormant account)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Comment on attachment 707481 [details] [diff] [review]
> fix v1
> 
> jduell granted r+ over IRC, so I pushed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/76dea17faf44

thanks jduell + dholbert :)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/76dea17faf44