1126790 - MainThreadIOLogger.cpp:50:7: warning: ‘mozilla::MainThreadIOLoggerImpl’ has a field ‘mozilla::MainThreadIOLoggerImpl::mObservations’ whose type uses the anonymous namespace [enabled by default]

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Daniel Holbert [:dholbert]]

Build warning with clang 3.6 (and earlier versions of clang as well, IIRC), when building current mozilla-central:
{
xpcom/build/MainThreadIOLogger.cpp:50:7: warning: ‘mozilla::MainThreadIOLoggerImpl’ has a field ‘mozilla::MainThreadIOLoggerImpl::mObservations’ whose type uses the anonymous namespace [enabled by default]
}

Comment 1

[Daniel Holbert [:dholbert]]

As noted in similar bug 946405 & bug 904148, we can fix this by either moving the anonymous-namespace-thing outside of the anonymous namespace, or we can move its usage *into* the anonymous namespace.

Patch coming up to do the latter.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Daniel Holbert [:dholbert]]

Sorry, I said in comment 0 that this was a clang warning, but I meant g++ 4.8. (I forgot that this particular local build (where I hit this warning) is configured to use gcc, instead of my default, clang.)

Comment 4

[Nathan Froyd [:froydnj]]

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

Review of attachment 8555874 [details] [diff] [review]:
-----------------------------------------------------------------

I'm kind of curious the situation this warning is protecting against.

Comment 5

[Daniel Holbert [:dholbert]]

IIUC:
 - stuff in the anonymous namespace can't be seen outside of this compilation unit
 - stuff *outside* the anonymous namespace *can* be seen outside of this compilation unit
 - So, if something outside the anonymous namespace uses something inside the anonymous namespace, [...bad stuff, not sure...]

Comment 6

[Daniel Holbert [:dholbert]]

Actually I think it's something like this:
 - All of our .cpp files are now "header files" (via unified compilation)
 - If a header file has an anonymous namespace, then each compilation unit that
includes that header puts that anonymous-namespaced-stuff in their *private,
per-compilation-unit* anonymous namespace
 - But that doesn't happen for stuff outside of the anonymous namespace.
 - So potentially, if we had this scenario in a header that's included in
varoius places, then you could end up with a class whose member-var has a
different type in each compilation unit where it's compiled.

The bad scenario that GCC is watching out for is:

foo.h:
  namespace {
    class AnonNamespaceThing {
    };
  }
  class NormalThing {
    AnonNamespaceThing mAnon;
  };

a.cpp:
  #include foo.h
  [code that uses NormalThing]

b.cpp:
  #include foo.h
  [other code that uses NormalThing]

And if some function in a.cpp passes a NormalThing to a function in b.cpp, then
technically they'll each have a different concept of what NormalThing::mAnon is.

Comment 7

[Daniel Holbert [:dholbert]]

(And, to close out the thought: if we respond to the warning by moving NormalThing into the anonymous namespace [as my patch here does, in our version of this parable], then a.cpp and b.cpp will each have differently-typed NormalThings [defined in different anonymous namespaces) and wouldn't be able to pass them back & forth and get weird under-the-covers type mismatches.)

(None of this really matters for us because we don't #include the "header" MainThreadIOLogger.cpp in multiple places.)

Comment 8

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c665e2057216

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c665e2057216