Closed
Bug 1396850
Opened 7 years ago
Closed 7 years ago
clang -Wundefined-var-template warnings in JNI and widget code
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(1 file, 1 obsolete file)
4.58 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
clang complains thusly: In file included from /opt/build/froydnj/build-android/widget/android/Unified_cpp_widget_android1.cpp:47: In file included from /home/froydnj/src/gecko-dev.git/widget/android/nsWidgetFactory.cpp:14: /home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:136:44: warning: instantiation of variable 'nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName' required here, but no definition is available [-Wundefined-var-template] , mWindowLock(NativePtr<Impl>::sName) ^ /home/froydnj/src/gecko-dev.git/widget/android/GeckoEditableSupport.h:173:11: note: in instantiation of member function 'nsWindow::WindowPtr<mozilla::widget::GeckoEditableSupport>::WindowPtr' requested here , mWindow(aPtr, aWindow) ^ /home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:84:27: note: forward declaration of template entity is here static const char sName[]; ^ /home/froydnj/src/gecko-dev.git/widget/android/nsWindow.h:136:44: note: add an explicit instantiation declaration to suppress this warning if 'nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName' is explicitly instantiated in another translation unit , mWindowLock(NativePtr<Impl>::sName) ^ In file included from /opt/build/froydnj/build-android/widget/android/Unified_cpp_widget_android0.cpp:2: In file included from /home/froydnj/src/gecko-dev.git/widget/android/ANRReporter.cpp:6: In file included from /home/froydnj/src/gecko-dev.git/widget/android/ANRReporter.h:9: In file included from /home/froydnj/src/gecko-dev.git/widget/android/fennec/FennecJNINatives.h:10: In file included from /opt/build/froydnj/build-android/dist/include/FennecJNIWrappers.h:10: /opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:245:55: warning: instantiation of variable 'mozilla::jni::ObjectBase<mozilla::jni::TypedObject<jstring>, _jstring *>::name' required here, but no definition is available [-Wundefined-var-template] const jclass cls = GetClassRef(mEnv, Cls::name); ^ /opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:957:53: note: in instantiation of member function 'mozilla::jni::Context<mozilla::jni::TypedObject<jstring>, _jstring *>::ClassRef' requested here typename Cls::Context(env, nullptr).ClassRef(), ^ /home/froydnj/src/gecko-dev.git/widget/android/EventDispatcher.cpp:228:35: note: in instantiation of function template specialization 'mozilla::jni::TypedObject<_jobjectArray *>::New<mozilla::jni::TypedObject<jstring> >' requested here auto keys = jni::ObjectArray::New<jni::String>(length); ^ /opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:317:23: note: forward declaration of template entity is here static const char name[]; ^ /opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:245:55: note: add an explicit instantiation declaration to suppress this warning if 'mozilla::jni::ObjectBase<mozilla::jni::TypedObject<jstring>, _jstring *>::name' is explicitly instantiated in another translation unit const jclass cls = GetClassRef(mEnv, Cls::name); I tried to rewrite the first bit of problematic code, but I didn't see a good way to resolve the circular dependence between NativePtr and WindowPtr. And the second one defeats me completely. Jim, are you able to produce patches that can fix the problem these warnings are pointing out?
Flags: needinfo?(nchen)
Comment 1•7 years ago
|
||
I think for the first warning, we should move the definition at [1] to inside nsWindow.cpp, where we instantiate other instances of `sName` (e.g. [2]) [1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/GeckoEditableSupport.cpp#29 [2] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/nsWindow.cpp#736 For the second warning, I think the proper fix is to copy the `name[]` definitions from [3] to Ref.h, and make them declarations. For example, somewhere around [4], we would have a list of > template<> const char ObjectBase<Object, jobject>::name[]; > template<> const char ObjectBase<TypedObject<jstring>, jstring>::name[]; > ... [3] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Utils.cpp#52 [4] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Refs.h#354 Let me know if you want me to write the patch, though I think you are in a better position to test if the patch actually works or not :)
Flags: needinfo?(nchen)
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #1) > I think for the first warning, we should move the definition at [1] to > inside nsWindow.cpp, where we instantiate other instances of `sName` (e.g. > [2]) > > [1] > http://searchfox.org/mozilla-central/rev/ > f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/GeckoEditableSupport. > cpp#29 > [2] > http://searchfox.org/mozilla-central/rev/ > f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/nsWindow.cpp#736 It's not clear to me why this would work. The error messages suggest that the errors are coming from the headers themselves, without any .cpp involvement. > For the second warning, I think the proper fix is to copy the `name[]` > definitions from [3] to Ref.h, and make them declarations. For example, > somewhere around [4], we would have a list of > > > template<> const char ObjectBase<Object, jobject>::name[]; > > template<> const char ObjectBase<TypedObject<jstring>, jstring>::name[]; > > ... > > [3] > http://searchfox.org/mozilla-central/rev/ > f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Utils.cpp#52 > [4] > http://searchfox.org/mozilla-central/rev/ > f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/widget/android/jni/Refs.h#354 I will have to think about this; we don't want the definitions in the header itself, certainly.
Comment 3•7 years ago
|
||
Hm I think you're right. IIUC the warning happens when we try to instantiate `nsWindow` inside nsWidgetFactory.cpp (and therefore `nsWindow::NativePtr<GeckoEditableSupport>::sName`), but `sName` is not declared within nsWidgetFactory's translation unit (note that we declare the templated `sName` but not the instantiated `sName`). So the solution is to somehow declare the instantiated `sName` in nsWidgetFactory's translation unit. We can add declarations to nsWindow.h, or we can add definitions to nsWidgetFactory.cpp. Either way should work.
Assignee | ||
Comment 4•7 years ago
|
||
Doing this avoids -Wundefined-var-template warnings with clang. This warning is mostly harmless (clang is trying to tell you a linker error might be awaiting you later), but being careful with this might make using C++ modules easier somewhere down the line.
Attachment #8904798 -
Flags: review?(nchen)
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8904798 [details] [diff] [review] explicitly declare static members of templates prior to use Review of attachment 8904798 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/moz.configure/warnings.configure @@ +92,5 @@ > # we don't want our builds held hostage when a platform-specific API > # becomes deprecated. > check_and_add_gcc_warning('-Wno-error=deprecated-declarations') > > +#check_and_add_gcc_warning('-Wno-error=undefined-var-template') Argh, ignore this part, sorry. :(
Comment 6•7 years ago
|
||
Comment on attachment 8904798 [details] [diff] [review] explicitly declare static members of templates prior to use Review of attachment 8904798 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/jni/Refs.h @@ +313,5 @@ > using GlobalRef = jni::GlobalRef<Cls>; > using Param = const Ref&; > > static const CallingThread callingThread = CallingThread::ANY; > + static const char* const name; The declarations should be enough? Is it necessary to change the type here? You would need to change all the types in GeneratedJNIWrappers/FennecJNIWrappers, and the annotation processor as well. ::: widget/android/nsWindow.h @@ -132,5 @@ > > - WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow) > - : mPtr(aPtr) > - , mWindow(aWindow) > - , mWindowLock(NativePtr<Impl>::sName) Might be more clear to add `NativePtr<Impl>::GetName()` and use that, then I think you can keep `WindowPtr::WindowPtr` inlined.
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Jim Chen [:jchen] [:darchons] from comment #6) > ::: widget/android/jni/Refs.h > @@ +313,5 @@ > > using GlobalRef = jni::GlobalRef<Cls>; > > using Param = const Ref&; > > > > static const CallingThread callingThread = CallingThread::ANY; > > + static const char* const name; > > The declarations should be enough? Is it necessary to change the type here? > You would need to change all the types in > GeneratedJNIWrappers/FennecJNIWrappers, and the annotation processor as well. Ugh, probably didn't notice that because I was running a debug build. What "declarations should be enough" are you referring to? > ::: widget/android/nsWindow.h > @@ -132,5 @@ > > > > - WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow) > > - : mPtr(aPtr) > > - , mWindow(aWindow) > > - , mWindowLock(NativePtr<Impl>::sName) > > Might be more clear to add `NativePtr<Impl>::GetName()` and use that, then I > think you can keep `WindowPtr::WindowPtr` inlined. Accessing `sName` in `GetName()` has the same issue with regards to the warning.
Assignee | ||
Comment 8•7 years ago
|
||
Let's try this version. Botond helped me get the template declarations right, so we don't have to change any of the datatypes involved.
Attachment #8904806 -
Flags: review?(nchen)
Assignee | ||
Updated•7 years ago
|
Attachment #8904798 -
Attachment is obsolete: true
Attachment #8904798 -
Flags: review?(nchen)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → nfroyd
Comment 9•7 years ago
|
||
Comment on attachment 8904806 [details] [diff] [review] explicitly declare static members of templates prior to use Review of attachment 8904806 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/jni/Refs.h @@ +374,5 @@ > +template<> const char ObjectBase<TypedObject<jobjectArray>, jobjectArray>::name[]; > + > +class ByteBuffer; > + > +template<> const char ObjectBase<ByteBuffer, jobject>::name[]; I would move this to right after where we define `ByteBuffer` below, but nbd. ::: widget/android/nsWindow.h @@ +354,5 @@ > +template<> const char nsWindow::NativePtr<mozilla::widget::GeckoEditableSupport>::sName[]; > +template<> const char nsWindow::NativePtr<nsWindow::NPZCSupport>::sName[]; > + > +template<class Impl> > +nsWindow::WindowPtr<Impl>::WindowPtr(NativePtr<Impl>* aPtr, nsWindow* aWindow) > Accessing `sName` in `GetName()` has the same issue with regards to the warning. So I meant adding `NativePtr::GetName()` and putting its definition down here instead of `nsWindow::WindowPtr`, so `nsWindow::WindowPtr` can stay inlined, but nbd either.
Attachment #8904806 -
Flags: review?(nchen) → review+
Comment 10•7 years ago
|
||
Pushed by nfroyd@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dd3dbdc735 explicitly declare static members of templates prior to use; r=darchons
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e3dd3dbdc735
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•