Closed Bug 525172 Opened 15 years ago Closed 15 years ago

IPDL: Generate .cpp files with method definitions

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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
Attached patch v1 (obsolete) — Splinter Review
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)
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)
I've noticed that the v2 patch ends up invoking the IPDL compiler more than once.  Is there any way to void that?
(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 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-
(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.
Attachment #412371 - Attachment is obsolete: true
Attachment #413492 - Flags: review?(benjamin)
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+
Pushed http://hg.mozilla.org/projects/electrolysis/rev/a006c4859410
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 1468272
You need to log in before you can comment on or make changes to this bug.