Closed Bug 921563 Opened 10 years ago Closed 10 years ago

build ipc/ipdl/ in "unity" mode


(Core :: IPC, defect)

Not set





(Reporter: froydnj, Unassigned)



(4 files, 2 obsolete files)

      No description provided.
Every IPDL C++ class contains a bunch of typedefs.

Every IPDL C++ source file contains a bunch of typedefs and using statements
for the exact same types.

Why is that?

Because the class itself is not in scope for name lookup of return types of C++
methods.  This limitation is annoying, but it makes sense.  The typedefs and
using statements therefore exist so we can be a little sloppy about return

Let's stop being sloppy and polluting the global namespace inside these
files.  Less pollution makes it easier to smash the IPDL files together for
unified compilation.  One could do this by qualifying all necessary return
types...or we could just use the C++0x late return syntax, which guarantees the
class *is* in scope for name lookup.  I like this version a lot better.

(Light testing with MSVC indicates that it supports late return syntax.  Hooray!)
Attachment #811254 - Flags: review?(bent.mozilla)
Now that everything's qualified, we can get rid of these.
Attachment #811255 - Flags: review?(bent.mozilla)
Thanks to gps's review, implementing the actual unification bits was easy!

Thanks to gps's tests, making sure the tests passed was annoying! :)
Attachment #811257 - Flags: review?(gps)
Speedup for compiling ipc/ipdl/ was only about 2x on my machine; I'd be interested to hear about other people's results.

And Ehsan, since you so boldly reviewed the last set of patches, you can feel free to review these, too! :)
Attachment #811254 - Flags: review?(bent.mozilla) → review+
Attachment #811255 - Flags: review?(bent.mozilla) → review+
I should start adding ":bent" to my bugzilla name.  ;-)
Try suggests that all our compilers are happy:

The Windows and OSX builds didn't finish, but the logs show that they compiled the files without any issues. \o/
Happy tears!
Comment on attachment 811257 [details] [diff] [review]
part 3 - compile ipc/ipdl/ in "unity" mode

Review of attachment 811257 [details] [diff] [review]:

I won't say this that often, but I think the test coverage is a bit too thorough. You probably really only need to verify the lines containing "UnifiedProtocols0.cpp" match expectations. Everything else seems to be excessive testing of add_unified_build_rules()'s implementation details.
Attachment #811257 - Flags: review?(gps) → review+
Did you mean something along these lines with your review comments?
Attachment #812071 - Flags: review?(gps)
Same as the old part three, but applied on top of the simplified tests.  Carrying
over r+.
Attachment #811257 - Attachment is obsolete: true
Attachment #812073 - Flags: review+
Comment on attachment 812071 [details] [diff] [review]
part 3 - simplify test_ipdl_sources

Review of attachment 812071 [details] [diff] [review]:

I was referring to all the inline comments from the unified make file boilerplate. I'm not sure what happened to them since I don't see them in this diff. You can roll this into the other patch if you want - no harm in it.

::: python/mozbuild/mozbuild/test/backend/
@@ +476,5 @@
>          ]
> +
> +        found = [str for str in lines if (str.startswith('ALL_IPDLSRCS') or
> +                                          str.startswith('CPPSRCS') or
> +                                          str.startswith('IPDLDIRS'))]

IIRC str.startswith() accepts a tuple of strings to match against.
Attachment #812071 - Flags: review?(gps)
Right, the big comment block is nowhere to be seen, because I'm not testing for
that anymore.  (The meat of the changes are not in part 4.)  I'm simply making
this change so that the actual test change in part 4 is rather small--the extra
unified build comments, rules, variables, etc., won't even be checked now.  Easier
(sort of!) to figure out what's going on.

Didn't know about passing tuples to startswith; fixed in this version.
Attachment #812071 - Attachment is obsolete: true
Attachment #812090 - Flags: review?(gps)
Attachment #812090 - Flags: review?(gps) → review+
You need to log in before you can comment on or make changes to this bug.