clang gives errors like: In file included from /home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.cpp:6: In file included from /home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.h:9: /home/froydnj/src/gecko-dev.git/widget/android/GeneratedJNINatives.h:135:40: error: no member named 'NotifyCursorDone' in 'mozilla::SmsManager' ::template Wrap<&Impl::NotifyCursorDone>), ~~~~~~^ /home/froydnj/src/gecko-dev.git/dom/mobilemessage/android/SmsManager.h:13:27: note: in instantiation of template class 'mozilla::widget::GeckoSmsManager::Natives<mozilla::SmsManager>' requested here class SmsManager : public widget::GeckoSmsManager::Natives<SmsManager> And SmsManager, of course, has a NotifyCursorDone method: http://dxr.mozilla.org/mozilla-central/source/dom/mobilemessage/android/SmsManager.cpp#385 I feel like I have run into this problem before, but I don't remember what the resolution was. Or maybe this was a bug in clang that was supposed to be fixed in subsequent releases, but the fix hasn't made it into the NDK yet? Do you remember?

See bug 1163171 comment 4. Guess this was an old clang problem after all.

OK, multiple versions of clang complain about essentially the problem demonstrated below: #include <inttypes.h> typedef struct { const char* name; const char* signature; void* fnPtr; } JNINativeMethod; class GeckoSmsManager { public: template<class Impl> class Natives; }; template<class Impl> class GeckoSmsManager::Natives { public: static constexpr JNINativeMethod methods[] = { { "name", "signature", reinterpret_cast<void*>(&Impl::NotifyCursorDone) } }; }; template<class Impl> constexpr JNINativeMethod GeckoSmsManager::Natives<Impl>::methods[]; class SmsManager : public GeckoSmsManager::Natives<SmsManager> { public: static void NotifyCursorDone(int32_t); }; They complain that SmsManager doesn't have any member named "NotifyCursorDone", when it clearly does. But GCC, MSVC, and icc (icc checked with gcc.godbolt.org) all compile it without a fuss. Is clang correct here, or is this a bug that needs to be fixed? (I haven't checked with the most-up-to-date clang yet, so it's possible it *is* fixed...)

I can make a pretty good guess as to why clang is giving the error: - While processing the base clause of SmsManager, it instantiates Natives<SmsManager> (to determine its size). - While instantiating Natives<SmsManager>, it tries to instantiate the initializer of the static constexpr data member. - That initializer refers to Impl::NotifyCursorDone, with [Impl = SmsManager]. - However, at this point SmsManager isn't complete yet (we're still processing its base clause!), so it errors out. I'll further hypothesize that clang is right, and the other compilers are just lenient, deferring the instantiation of the initializer until it's actually needed. I haven't confirmed this, though.

(Note that the size of the class can legitimately depend on the value of static constexpr data member, it just doesn't in this case. For example, if JNINativeMethod had a field "int foo", Natives could have a field of type "std::array<char, methods[0].foo>", and now computing the size of the class requires instantiating the constexpr's initializer. This is what makes me think that clang is right.)

If it's not important for Natives::methods to be constexpr, making it not constexpr and moving its initializer to its definition works around the issue.

After doing some standard reading, I believe clang is correct. I'll explain my reasoning. To start with, it's clear that processing the base clause "public GeckoSmsManager::Natives<SmsManager>" requires implicit instantiation of GeckoSmsManager::Natives<SmsManager>. Now we consult [temp.inst] p1: [...] The implicit instantiation of a class template specialization causes the implicit instantiation of the declarations, but not of the definitions, default arguments, or exception-specifications of the class member functions, member classes, scoped member enumerations, static data members and member templates; [...] So implicit instantiation of GeckoSmsManager::Natives<SmsManager> causes implicit instantiation of the declaration of GeckoSmsManager::Natives<SmsManager>::methods, but not of its definition. So the question is, is the initializer considered part of the declaration or part of the definition? I believe [class.static.data] p3 sheds light on this: [...] A static data member of literal type can be declared in the class definition with the constexpr specifier; if so, its declaration shall specify a brace-or-equal-initializer [...]. The member shall still be defined in a namespace scope if it is odr-used in the program and the namespace scope definition shall not contain an initializer. I interpret that as meaning that the initializer, when it appears inside the class definition like this, is considered part of the declaration and not the definition, and so is subject to implicit instantiation.

Thanks for the explanation, Botond. Looks like we have some fixing to do; I have patches.

clang complains that a constexpr definition of methods[] cannot refer to members of the incomplete Impl template parameter, and rightly so. Making these const is sufficient for our purposes, and that enables us to move the declaration outside of the class, where it will be instantiated lazily (presumably at the point when |Impl| is a complete class definition). We also need to declare the length of methods[], as other parts of the code require knowing the length of methods[] at compile time.

clang complains about specializations of NativeStubImpl not being able to see the private constructor of ProxyNativeCall. While ProxyNativeCall includes a friend declaration for NativeStubImpl, it's not obvious *which* NativeStubImpl is being friended, as NativeStubImpl hasn't been forward declared and exists in a completely separate namespace. Forward declaring NativeStubImpl and adjusting the friend declaration makes everything more correct.

clang complains that it's unable to instantiate this template because the functions being passed as arguments are MOZ_JNICALL, while the actual parameter to the function has no such attribute. Adding the attribute makes everything line up.

Comment on attachment 8759654 [details] [diff] [review] part 1 - make generated Natives<>::methods[] const instead of constexpr Review of attachment 8759654 [details] [diff] [review]: ----------------------------------------------------------------- LGTM with nits. ::: build/annotationProcessors/CodeGenerator.java @@ +34,5 @@ > > public CodeGenerator(ClassWithOptions annotatedClass) { > this.cls = annotatedClass.wrappedClass; > this.clsName = annotatedClass.generatedName; > + this.numNativesInits = 0; Don't need to init to 0. @@ +343,5 @@ > "\n" + > "\n" + > " mozilla::jni::MakeNativeMethod<" + traits + ">(\n" + > " mozilla::jni::NativeStub<" + traits + ", Impl>\n" + > " ::template Wrap<&Impl::" + info.wrapperName + ">)"); Reduce indentation of these lines by 4 spaces. @@ +569,4 @@ > "};\n" + > "\n" + > "template<class Impl>\n" + > + "const JNINativeMethod " + clsName + "::Natives<Impl>::methods[] = {" + nativesInits + "\n};" + Put "};" on separate line with new line at the end, i.e. > natives.append( > ... > "const JNINativeMethod " + clsName + "::Natives<Impl>::methods[] = {" + nativesInits + '\n' + > "};\n" + > "\n");

Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a215312a111 part 1 - make generated Natives<>::methods[] const instead of constexpr; r=darchons https://hg.mozilla.org/integration/mozilla-inbound/rev/9c355864809b part 2 - more explicit friending between NativeStubImpl and ProxyNativeCall; r=darchons https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd9cb917c45 part 3 - add MOZ_JNICALL attribute to MakeNativeMethod's argument; r=darchons

https://hg.mozilla.org/mozilla-central/rev/6a215312a111 https://hg.mozilla.org/mozilla-central/rev/9c355864809b https://hg.mozilla.org/mozilla-central/rev/4cd9cb917c45