Closed
Bug 826979
Opened 11 years ago
Closed 11 years ago
jsfriendapi.h conflict with qobjectdefs.h slots defines
Categories
(Core Graveyard :: Widget: Qt, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla21
People
(Reporter: romaxa, Assigned: romaxa)
References
Details
Attachments
(1 file, 3 obsolete files)
7.85 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
Recent change in bug 816421 added connection between Qt qobjectdefs.h and jsfriendapi.h c++ -o nsWidgetFactory.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include config/gcc_hidden.h -DMOZ_GLUE_IN_PROGRAM -DXPCOM_TRANSLATE_NSGM_ENTRY_POINT=1 -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES -DOS_POSIX=1 -DOS_LINUX=1 -D_IMPL_NS_WIDGET -D_BSD_SOURCE -Iipc/chromium/src -Iipc/glue -I../../ipc/ipdl/_ipdlheaders -Iwidget/xpwidgets -Iwidget/qt/faststartupqt -Iwidget/qt -Iwidget/qt -I. -I../../dist/include -Iobj-xr-qt/dist/include/nspr -Iobj-xr-qt/dist/include/nss -Iwidget/qt/../shared/x11 -Iwidget/qt/../shared -fPIC -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wcast-align -Wno-long-long -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -pipe -DDEBUG -D_DEBUG -DTRACING -g -Os -freorder-blocks -fno-omit-frame-pointer -DQT_SHARED -I/usr/include/qt4 -I/usr/include/qt4/QtGui -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtNetwork -I/usr/include/qt4/QtOpenGL -Iobj-xr-qt/dist/include/cairo -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MF .deps/nsWidgetFactory.o.pp widget/qt/nsWidgetFactory.cpp In file included from ../../dist/include/mozilla/dom/DOMJSClass.h:10:0, from ../../dist/include/mozilla/dom/DocumentBinding.h:8, from ../../dist/include/nsIDocument.h:27, from ../../dist/include/nsIContent.h:10, from widget/xpwidgets/nsBaseDragService.h:15, from widget/qt/nsDragService.h:11, from widget/qt/nsWidgetFactory.cpp:23: ../../dist/include/jsfriendapi.h:337:30: error: expected unqualified-id before ‘;’ token ../../dist/include/jsfriendapi.h: In member function ‘JS::Value& js::shadow::Object::slotRef(size_t) const’: ../../dist/include/jsfriendapi.h:349:27: error: expected ‘,’ before ‘-’ token ../../dist/include/jsfriendapi.h:349:27: error: expected identifier before ‘-’ token ../../dist/include/jsfriendapi.h: In lambda function: ../../dist/include/jsfriendapi.h:349:36: error: expected ‘{’ before ‘;’ token ../../dist/include/jsfriendapi.h: In member function ‘JS::Value& js::shadow::Object::slotRef(size_t) const’: ../../dist/include/jsfriendapi.h:349:36: error: invalid initialization of non-const reference of type ‘JS::Value&’ from an rvalue of type ‘js::shadow::Object::slotRef(size_t) const::<lambda()>’ In file included from ../../dist/include/xpcpublic.h:12:0, from ../../dist/include/mozilla/dom/DOMJSProxyHandler.h:12, from ../../dist/include/mozilla/dom/DocumentBinding.h:9, from ../../dist/include/nsIDocument.h:27, from ../../dist/include/nsIContent.h:10, from widget/xpwidgets/nsBaseDragService.h:15, from widget/qt/nsDragService.h:11, from widget/qt/nsWidgetFactory.cpp:23: ../../dist/include/js/MemoryMetrics.h: At global scope: ../../dist/include/js/MemoryMetrics.h:39:5: error: declaration does not declare anything [-fpermissive] ../../dist/include/js/MemoryMetrics.h: In member function ‘void JS::ObjectsExtraSizes::add(JS::ObjectsExtraSizes&)’: ../../dist/include/js/MemoryMetrics.h:51:36: error: expected unqualified-id before ‘+=’ token ../../dist/include/js/MemoryMetrics.h:51:50: error: expected unqualified-id before ‘;’ token make[1]: *** [nsWidgetFactory.o] Error 1
Assignee | ||
Comment 1•11 years ago
|
||
we need slots defines only for moc files compilation. that is related only to few files in Qt port
Assignee | ||
Comment 2•11 years ago
|
||
Simple and single place is to undefine slots which are coming from Qt includes. Another possible way is to rename slots to something else
Attachment #698227 -
Attachment is obsolete: true
Attachment #698236 -
Flags: review?(bzbarsky)
Comment 3•11 years ago
|
||
Comment on attachment 698236 [details] [diff] [review] Undef possibly defined slots before using jsfriendapi.h Would it make more sense to move DOMJSClass.h out of the DocumentBinding header (replacing it with a forward-declare)?
Comment 4•11 years ago
|
||
Comment on attachment 698236 [details] [diff] [review] Undef possibly defined slots before using jsfriendapi.h Looks like not easily. We'd have to uninline a bunch of stuff. This really needs review from a JS peer type person. :(
Attachment #698236 -
Flags: review?(bzbarsky) → review?(jwalden+bmo)
Comment 5•11 years ago
|
||
From IRC: <reuben> Waldo, can't you just define QT_NO_KEYWORDS globally and use Q_SLOTS/Q_SIGNALS/Q_EMIT in the files that use them? <reuben> Waldo, or rather, wouldn't that be easier? <reuben> Waldo, yea, grepping the qt files only shows 4 uses of slots and 1 of signals, you can use Q_SLOTS/Q_SIGNALS <reuben> Waldo, also Q_EMIT Is there a reason why adding #define QT_NO_KEYWORDS to mozilla-config.h.in and js/src/js-confdefs.h.in and doing what's suggested above wouldn't work? That seems like the best solution by far to me.
Comment 6•11 years ago
|
||
Comment on attachment 698236 [details] [diff] [review] Undef possibly defined slots before using jsfriendapi.h Review of attachment 698236 [details] [diff] [review]: ----------------------------------------------------------------- This is include-order-fragile, so I don't think this can fly just quite on its own, regardless of the answer to the previous comment.
Attachment #698236 -
Flags: review?(jwalden+bmo) → review-
Assignee | ||
Comment 7•11 years ago
|
||
Yep, reuben proposal works good and seems much more stable solution in long term. this is now mostly DONTBUILD fix, but just in case would ask glandium.
Attachment #698236 -
Attachment is obsolete: true
Attachment #701163 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 701163 [details] [diff] [review] Use reuben proposal which works good ups forgot netwerk part
Attachment #701163 -
Attachment is obsolete: true
Attachment #701163 -
Flags: review?(mh+mozilla)
Comment 9•11 years ago
|
||
I'd be wary of DONTBUILDing, actually, because I believe we have some compilers/architectures where AC_DEFINEs are #defined *after* a command-line include of a system header or two.
Comment 10•11 years ago
|
||
...and if the system header (system-ish, at least) had Qt #includes, it might be too late to #define QT_NO_KEYWORDS, to be precise.
Assignee | ||
Comment 11•11 years ago
|
||
This most likely it. will wait until full build compilation finished
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 701171 [details] [diff] [review] Define QT_NO_KEYWORDS and use Q_* macro instead slots/signals/emit What would be the best place to define it in this case? I compiled it locally and it seems to work, mainly it is needed for dom/plugins, netwerk, widget/qt
Attachment #701171 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 13•11 years ago
|
||
Tested latest patch and it works ok, (compiled it on desktop, Mer, Harmattan)
Assignee | ||
Comment 14•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=b1689b11ebf3
Comment 15•11 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9) > I'd be wary of DONTBUILDing, actually, because I believe we have some > compilers/architectures where AC_DEFINEs are #defined *after* a command-line > include of a system header or two. AC_DEFINEs are set in mozilla-config.h, and it's the first -include'd file.
Updated•11 years ago
|
Attachment #701171 -
Flags: review?(mh+mozilla) → review+
Comment 16•11 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #15) > AC_DEFINEs are set in mozilla-config.h, and it's the first -include'd file. Not necessarily. b2g uses CXXFLAGS to supply an extra -include, and until bug 812218 is fixed, that -include precedes the -include of mozilla-config.h. I discovered this when writing a patch for bug 730805 -- a patch that didn't work for b2g specifically because of this earlier -include. Of course, b2g wouldn't use Qt, but I have little confidence in ports not backsliding on this point, so long as it's possible to insert an -include before mozilla-config.h. We give a port that rope, and eventually they'll tie a noose with it. Which is why I'd really like to see bug 812218 fixed, but I don't have the time to debug a PGO failure triggered by commandline rearrangement right now.
Comment 17•11 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16) > Of course, b2g wouldn't use Qt, but I have little confidence in ports not > backsliding on this point, so long as it's possible to insert an -include > before mozilla-config.h. It's always and will always be possible to do so, even with bug 812218 fixed. For example, one could set CXX="g++ -include foo", and there's not much that can be done there, except for actually filtering CXX, and there's plenty occasions screwing up when doing that, so it's better not to. B2G is a special (bad) case, it's not worth worrying about footguns in other ports because of it.
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed46fb258e53
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ed46fb258e53
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•