921563 - build ipc/ipdl/ in "unity" mode

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - declare return types in IPDL C++ with C++0x late return syntax

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
types.

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!)

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - delete typedefs and using statements from global scope in IPDL C++ files

Now that everything's qualified, we can get rid of these.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - compile ipc/ipdl/ in "unity" mode

Thanks to gps's review, implementing the actual unification bits was easy!

Thanks to gps's tests, making sure the tests passed was annoying! :)

Comment 4

[Nathan Froyd [:froydnj]]

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! :)

Comment 5

[(no longer active)]

I should start adding ":bent" to my bugzilla name.  ;-)

Comment 6

[Nathan Froyd [:froydnj]]

Try suggests that all our compilers are happy: https://tbpl.mozilla.org/?tree=Try&rev=0cb2c3a38d6a

The Windows and OSX builds didn't finish, but the logs show that they compiled the files without any issues. \o/

Comment 7

[(no longer active)]

Happy tears!

Comment 8

[Gregory Szorc [:gps]]

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.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 3 - simplify test_ipdl_sources

Did you mean something along these lines with your review comments?

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 4 - compile ipc/ipdl/ in "unity" mode

Same as the old part three, but applied on top of the simplified tests.  Carrying
over r+.

Comment 11

[Gregory Szorc [:gps]]

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/test_recursivemake.py
@@ +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.

Comment 12

[Nathan Froyd [:froydnj]]

Attachment: part 3 - simplify test_ipdl_sources

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.

Comment 13

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/108da2233a8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/357d8cab2d45
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5bb0e56b6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0cea4c8eaa

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/108da2233a8f
https://hg.mozilla.org/mozilla-central/rev/357d8cab2d45
https://hg.mozilla.org/mozilla-central/rev/0a5bb0e56b6d
https://hg.mozilla.org/mozilla-central/rev/2a0cea4c8eaa
https://hg.mozilla.org/mozilla-central/rev/0fc6b2c6bd26