553846 - IPDL: Implement aggregate types

Status: RESOLVED FIXED

(Core :: IPC, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm writing a protocol in which I would find it useful to do

  [aggregate] Insert { Something x; Something where; }
  [aggregate] Remove { Something x; }
  union Operation { Insert; Remove; }

If there were only one Operation per message, this could be hacked around with

  Do(operation op, Something x, nullable Something y);

but in my case there may be many "operations" per message.  So I think IPDL needs an aggregate type.  The m.o. syntactically so far has been "look like C/C++" so "[aggregate]" needs to be "struct".

For an IPDL |struct Foo { X x; Y y; }|, I think we need to generate the following C++

  struct Foo {
    X x;
    Y y;
    Foo() { }
    Foo(const X& _x, const Y& _y) : x(_x), y(_y) { }
    Foo(const Foo& f) : x(f.x), y(f.y) { }
    Foo& operator=(const Foo& f) {
      x = f.x;  y = f.y;  return *this;
    }
  };

Luckily this is quite straightforward compared to IPDL unions.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement an IPDL "struct" type

Please let me know if you don't feel comfortable reviewing this.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Tests

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 444600 [details] [diff] [review]
Implement an IPDL "struct" type

Yesterday for some reason my mind wasn't processing this patch at all, but today the review was a breeze.

Regarding this suckage:

>+        sd.decl.type._sd = sd           # sucky
>+        newfields = [ ]

How about

          newfields = [ ]
          sd.decl.type.fields = newfields

since later (when pickling) you only use the .fields property of that struct declaration?  Do you ever reassign the fields property after

>+        sd.fields = newfields

a few lines down?

>+    def visitUnionType(self, t):
>+        if t in self.seen: return
>+        self.seen.add(t)
>+        self.maybeTypedef(t.fullname(), t.name())
>+
>+        return ipdl.type.TypeVisitor.visitUnionType(self, t)
>+
>+    def visitArrayType(self, t):
>+        return ipdl.type.TypeVisitor.visitArrayType(self, t)

Are array types not subject to the same single-definition rule?  Or are you guaranteed to call visitArrayType at most once?  Also, why no visitStructType method here?

>+    def implementStructPickling(self, structtype):
...
>+        sd = structtype._sd
...
>+        for f in sd.fields:

If you do as I suggested above, then this would become

          fields = structtype.fields
...
          for f in fields:

>+def p_StructFields(p):
>+    """StructFields : StructFields StructField ';'
>+                    | StructField ';'"""

Were you going to switch to right-recursion in general?  Or just where you had shift/reduce conflicts?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> (From update of attachment 444600 [details] [diff] [review])

Thanks for reviewing this!

> Regarding this suckage:
> 
> >+        sd.decl.type._sd = sd           # sucky
> >+        newfields = [ ]
> 
> How about
> 
>           newfields = [ ]
>           sd.decl.type.fields = newfields
> 
> since later (when pickling) you only use the .fields property of that struct
> declaration?  Do you ever reassign the fields property after
> 
> >+        sd.fields = newfields
> 
> a few lines down?
> 

The suckage is AST info leaking into the type system.  What you propose is a prettier way to leak that info ;).  The original need for this code was that serialization/deserialization code was generated from a place where the AST wasn't easily available, but now that that's been rewritten, the AST is available.  So I think what I can do here instead is set up a map of Type --> Node where required in the protocol-actor lowering class.

> >+    def visitUnionType(self, t):
> >+        if t in self.seen: return
> >+        self.seen.add(t)
> >+        self.maybeTypedef(t.fullname(), t.name())
> >+
> >+        return ipdl.type.TypeVisitor.visitUnionType(self, t)
> >+
> >+    def visitArrayType(self, t):
> >+        return ipdl.type.TypeVisitor.visitArrayType(self, t)
> 
> Are array types not subject to the same single-definition rule?  Or are you
> guaranteed to call visitArrayType at most once?  Also, why no visitStructType
> method here?
> 

Arrays don't get typedefs, they're generated as raw |nsTArray<Foo>|.  So it's only their basetype that needs to get the |self.seen()| check.  Nice catch on the struct thing, that's an oversight.

> >+def p_StructFields(p):
> >+    """StructFields : StructFields StructField ';'
> >+                    | StructField ';'"""
> 
> Were you going to switch to right-recursion in general?  Or just where you had
> shift/reduce conflicts?

Just where I was forced to by shift/reduce conflicts (the main ProtocolBody grammar's recursion).

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement an IPDL "struct" type, v2

Addressed comments.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement an IPDL "struct" type, v2

Oops, a typo fix went in the wrong patch.

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 445819 [details] [diff] [review]
Implement an IPDL "struct" type, v2

I am satisfied.  Thanks for answering my questions!

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/361074e4ab96
http://hg.mozilla.org/mozilla-central/rev/dc2985f8ad9f