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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(1 file, 3 obsolete files)

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
we need slots defines only for moc files compilation. that is related only to few files in Qt port
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #698227 - Flags: review+
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 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 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)
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 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-
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)
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)
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.
...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.
This most likely it. will wait until full build compilation finished
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)
Tested latest patch and it works ok, (compiled it on desktop, Mer, Harmattan)
(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.
Attachment #701171 - Flags: review?(mh+mozilla) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/ed46fb258e53
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.