Closed Bug 835715 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
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 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.
Attachment #707481 - Flags: review?(jduell.mcbugs) → review?(taras.mozilla)
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.
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. :)
> 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
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.
(where "that tweak" = "putting it adjacent to the #define rather than the #undef")
(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 :)
https://hg.mozilla.org/mozilla-central/rev/76dea17faf44
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.