Closed
Bug 525172
Opened 15 years ago
Closed 15 years ago
IPDL: Generate .cpp files with method definitions
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(1 file, 2 obsolete files)
37.74 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
The method signatures for actors are pretty stable, but the definitions fluctuate with IPDL bugfixes / feature enhancements. Each time this happens, even if one protocol in a protocol "tree" is affected, the entire tree (including other dependent C++ code) is rebuilt. I don't particularly care because I build on a really powerful machine. But as more and more people work on e10s code, and protocol trees get taller and bushier, the probability of strident complaint goes up.
Assignee | ||
Comment 1•15 years ago
|
||
The lack of C++ definitions is actually a real bug, not an enhancement. For example, if we have the protocol tree Parent | +--Brother | +--Sister and Brother wants to pass Sister actors in messages, while at the same time Sister wants to pass around Brother actors in messages, then the generated C++ will fail to compile in the current scheme. Brother needs the complete definition of Sister before he can pass Sister actors in messages, as does Sister for Brother. So we have a circular header dependency than needs to be broken by putting the method definitions that require the complete definitions of Brother/Sister into .cpp files.
Severity: enhancement → normal
Summary: IPDL: Consider generating .cpp files with method definitions → IPDL: Generate .cpp files with method definitions
Assignee | ||
Comment 2•15 years ago
|
||
r? to bsmedberg because this includes some build system hackery. (FWIW, I thought this would require some serious blood, sweat, tears, and ugly hackery. On the contrary, it quick and quite painless.)
Assignee: nobody → jones.chris.g
Attachment #412365 -
Flags: review?(benjamin)
Assignee | ||
Comment 3•15 years ago
|
||
v1 worked great, until I tried a full rebuild, upon which it fell down because of unexported headers that the IPDL sources relied on. v2 juggles some of those dependencies. Please let me know if there's a better way.
Attachment #412365 -
Attachment is obsolete: true
Attachment #412371 -
Flags: review?(benjamin)
Attachment #412365 -
Flags: review?(benjamin)
Assignee | ||
Comment 4•15 years ago
|
||
I've noticed that the v2 patch ends up invoking the IPDL compiler more than once. Is there any way to void that?
Assignee | ||
Comment 5•15 years ago
|
||
(In reply to comment #4) > I've noticed that the v2 patch ends up invoking the IPDL compiler more than > once. Is there any way to void that? s/void/avoid/
Comment 6•15 years ago
|
||
Comment on attachment 412371 [details] [diff] [review] v2, now with 125% more build system hackery >diff --git a/ipc/Makefile.in b/ipc/Makefile.in >-DIRS += testshell >+DIRS += \ >+ chromium \ >+ glue \ >+ $(NONE) >+ > TOOL_DIRS = app > > include $(topsrcdir)/config/rules.mk >+ >+export:: >+ @$(MAKE) -C ipdl export I don't understand all the gyrations that this directory is going through. Why can't it just be DIRS = chromium glue ipdl The comment in toolkit-tiers.mk is unclear as well: all the headers for a tier are exported before any of the .cpp are compiled, so the bit about not having all the headers means that something in the build is out of order, I think... >diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in >+ >+CPPSRCS := \ >+ $(ALL_IPDLSRCS:%.ipdl=$(DEPTH)/%Parent.cpp) \ >+ $(ALL_IPDLSRCS:%.ipdl=$(DEPTH)/%Child.cpp) \ >+ $(NULL) CPPSRCS must be bare filenames without directories, in order for dependencies to generate correctly. In this case I don't see any reason to have them being generated in $(DEPTH) instead of in this build directory, e.g. <objdir>/ipc/ipdl/PPluginModuleParent.cpp And then it would just be CPPSRCS = $(ALL_IPDLSRCS:%.ipdl=%Parent.cpp) etc... Also, please use = instead of := for all variables unless otherwise indicated, since occasionally rules.mk depends on that for some behavior. >+ --outcpp-dir=$(DEPTH) \ And then this would be, I think, --outcpp-dir=., except that should really be unnecessary. >diff --git a/ipc/ipdl/test/cxx/IPDLUnitTestTypes.h b/ipc/ipdl/test/cxx/IPDLUnitTestTypes.h >+#ifndef mozilla__ipdltest_IPDLUnitTestTypes_h >+#define mozilla__ipdltest_IPDLUnitTestTypes_h 1 Adding the extra "1" in the include guard is not standard and confused me for a sec, please remove it. >+ static bool Read(const Message* aMsg, void** aIter, paramType* aResult) >+ { >+ return (ReadParam(aMsg, aIter, &aResult->x) >+ && ReadParam(aMsg, aIter, &aResult->y) >+ && ReadParam(aMsg, aIter, &aResult->w) >+ && ReadParam(aMsg, aIter, &aResult->h)); >+ } Please put the && operator on the end of the line instead of the beginning.
Attachment #412371 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6) > (From update of attachment 412371 [details] [diff] [review]) > >diff --git a/ipc/Makefile.in b/ipc/Makefile.in > > >-DIRS += testshell > >+DIRS += \ > >+ chromium \ > >+ glue \ > >+ $(NONE) > >+ > > TOOL_DIRS = app > > > > include $(topsrcdir)/config/rules.mk > >+ > >+export:: > >+ @$(MAKE) -C ipdl export > > I don't understand all the gyrations that this directory is going through. Why > can't it just be > > DIRS = chromium glue ipdl > > The comment in toolkit-tiers.mk is unclear as well: all the headers for a tier > are exported before any of the .cpp are compiled, so the bit about not having > all the headers means that something in the build is out of order, I think... > The problem was that ipc/ipdl/ was in the xpcom tier, so actor .cpp builds were attempted before the gecko headers were exported. But based on what you say, I should be able to put ipc/ipdl in the gecko tier, right? > >diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in > > >+ > >+CPPSRCS := \ > >+ $(ALL_IPDLSRCS:%.ipdl=$(DEPTH)/%Parent.cpp) \ > >+ $(ALL_IPDLSRCS:%.ipdl=$(DEPTH)/%Child.cpp) \ > >+ $(NULL) > > CPPSRCS must be bare filenames without directories, in order for dependencies > to generate correctly. Nice catch! I debugged a nasty problem on Tuesday that was caused by this. > >diff --git a/ipc/ipdl/test/cxx/IPDLUnitTestTypes.h b/ipc/ipdl/test/cxx/IPDLUnitTestTypes.h > > >+#ifndef mozilla__ipdltest_IPDLUnitTestTypes_h > >+#define mozilla__ipdltest_IPDLUnitTestTypes_h 1 > > Adding the extra "1" in the include guard is not standard and confused me for a > sec, please remove it. > Oops, bad copy 'n paste.
Assignee | ||
Comment 8•15 years ago
|
||
Attachment #412371 -
Attachment is obsolete: true
Attachment #413492 -
Flags: review?(benjamin)
Comment 9•15 years ago
|
||
Comment on attachment 413492 [details] [diff] [review] v3, address comments >diff --git a/ipc/ipdl/Makefile.in b/ipc/ipdl/Makefile.in >+LOCAL_INCLUDES += -I$(DEPTH)/ipc/ipdl/_ipdlheaders I think this can just be LOCAL_INCLUDE += -I_ipdlheaders >diff --git a/ipc/ipdl/ipdl.py b/ipc/ipdl/ipdl.py >+op.add_option('-o', '--outcpp-dir', dest='cppdir', default='.', >+ help="""Director into which C++ sources will be generated spelling "Directory" >+log(1, 'Generated C++ sources will be generated relative to "%s"', cppdir) s/relative to/in/ for this line.
Attachment #413492 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 10•15 years ago
|
||
Pushed http://hg.mozilla.org/projects/electrolysis/rev/a006c4859410
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•