Last Comment Bug 521898 - IPDL should allow me to use a union in another protocol
: IPDL should allow me to use a union in another protocol
Status: RESOLVED FIXED
[soft][qa-]
:
Product: Core
Classification: Components
Component: IPC (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla16
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
: Bill McCloskey (:billm)
Mentors:
Depends on:
Blocks: 666693
  Show dependency treegraph
 
Reported: 2009-10-12 15:51 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2012-09-02 08:06 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
part 1: Hackily stuff away the AST in a more consistent way. We need it for structs/unions too because they may come from other TUs (8.03 KB, patch)
2012-06-04 14:58 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 2: Rename StructField .type to .typespec to avoid unintended "shadowing" (2.54 KB, patch)
2012-06-04 15:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 3: Rename ProtocolInclude to Include in preparation of more general usage (9.52 KB, patch)
2012-06-04 15:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 4: Make TranslationUnits namespaced things in preparation for headers, which don't have distinguished nodes to infer a C++ namespace from (3.43 KB, patch)
2012-06-04 15:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 5: Prepare for TU .protocol possibly being null (for headers) (13.35 KB, patch)
2012-06-04 15:03 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 6: Refactor the way structs/unions are declared (2.72 KB, patch)
2012-06-04 15:03 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice (6.12 KB, patch)
2012-06-04 15:03 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 8: Frontend support for .ipdlh (13.71 KB, patch)
2012-06-04 15:04 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 9: Backend support for .ipdlh (4.32 KB, patch)
2012-06-04 15:05 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
part 10: Add .ipdlh to the test harness and fix an annoying build warning (1.93 KB, patch)
2012-06-04 15:06 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review
Tests for bug 521898 (5.69 KB, patch)
2012-06-04 15:06 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bent.mozilla: review+
Details | Diff | Splinter Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2009-10-12 15:51:03 PDT
I'd like to be able to use a union defined in a subprotocol from its parent protocol after including the definition. The specific example here is Variant being used from PPluginInstance after being defined in (and included from) PPluginScriptableObject.
Comment 1 lekma 2011-08-11 02:08:01 PDT
Ben,

Did you ever get to work around this one?
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-24 17:16:18 PDT
Here's the general sketch of what I want here
 - add the directive |include Foo;|, which turns into a search for the file Foo.ipdlh as protocol specs would normally be searched.  (This might be a bit confusing with the C++-include statement |include "Bar.h";|.  We'll see.)
 - the decls in the .ipdlh are imported into the current tu's scope as per "global" decls in protocols
 - .ipdlh's can't declare protocols
 - .ipdlh's can't include protocol specs.  (We could relax this but I'd rather not.)
 - .ipdlh's can be recursively included
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-24 18:01:27 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
>  - .ipdlh's can't include protocol specs.  (We could relax this but I'd
> rather not.)

This will be allowed.  Let's see what happens.  It requires extra code in the compiler to forbid, anyway.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-25 00:57:42 PDT
Note to self: the frontend part of this is working.  The backend (codegen) part will need to "upgrade" all the dependent interesting types, wherever their origin.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-01 01:48:02 PDT
Done, working.  Splitting for review next time I get a bit of coding time (hopefully tomorrow).
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 14:58:46 PDT
Created attachment 629944 [details] [diff] [review]
part 1: Hackily stuff away the AST in a more consistent way.  We need it for structs/unions too because they may come from other TUs
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:01:10 PDT
Created attachment 629948 [details] [diff] [review]
part 2: Rename StructField .type to .typespec to avoid unintended "shadowing"
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:01:32 PDT
Created attachment 629949 [details] [diff] [review]
part 3: Rename ProtocolInclude to Include in preparation of more general usage
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:01:53 PDT
Created attachment 629950 [details] [diff] [review]
part 4: Make TranslationUnits namespaced things in preparation for headers, which don't have distinguished nodes to infer a C++ namespace from
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:03:00 PDT
Created attachment 629954 [details] [diff] [review]
part 5: Prepare for TU .protocol possibly being null (for headers)
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:03:23 PDT
Created attachment 629955 [details] [diff] [review]
part 6: Refactor the way structs/unions are declared
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:03:56 PDT
Created attachment 629958 [details] [diff] [review]
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:04:18 PDT
Created attachment 629959 [details] [diff] [review]
part 8: Frontend support for .ipdlh
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:05:47 PDT
Created attachment 629960 [details] [diff] [review]
part 9: Backend support for .ipdlh
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:06:14 PDT
Created attachment 629961 [details] [diff] [review]
part 10: Add .ipdlh to the test harness and fix an annoying build warning
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-04 15:06:39 PDT
Created attachment 629962 [details] [diff] [review]
Tests for bug 521898
Comment 17 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 14:42:17 PDT
Comment on attachment 629958 [details] [diff] [review]
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice

Review of attachment 629958 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/cxx/ast.py
@@ +369,5 @@
>          self.fromtype = fromtype
>          self.totypename = totypename
>  
> +    def __cmp__(self, o):
> +        return cmp(self.totypename, o.totypename)

Hm. You could theoretically have two typedefs that are in different namespaces that have the same name, right? Doesn't seem like this comparison will take that into account.
Comment 18 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 14:59:52 PDT
Comment on attachment 629958 [details] [diff] [review]
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice

Review of attachment 629958 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/lower.py
@@ +1261,5 @@
> +        if not isinstance(tu, TranslationUnit) and tu not in self.visitedTus:
> +            self.visitedTus.add(tu)
> +            ipdl.ast.Visitor.visitTranslationUnit(self, tu)
> +            TranslationUnit.upgrade(tu)
> +            self.typedefs[:] = sorted(list(self.typedefSet))

Can't this just be:

  self.typedefs = sorted(self.typedefSet)

Also, there are several places below where you add to |typedefSet| without updating |typedefs| list. Are you guaranteed to always have a |visitTranslationUnit()| call later to freshen your list?

@@ +1798,5 @@
>  
>      def visitShmemType(self, s):
>          if s in self.visited: return
>          self.visited.add(s)
> +        self.maybeTypedef('mozilla::ipc::Shmem', 'Shmem')

This change doesn't immediately look right. Did you mean to do this?
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 15:30:29 PDT
Comment on attachment 629959 [details] [diff] [review]
part 8: Frontend support for .ipdlh

Review of attachment 629959 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/parser.py
@@ +216,5 @@
> +    else:
> +        assert tu.filetype == 'header'
> +        # There's not really a canonical "thing" in headers.  So
> +        # somewhat arbitrarily use the namespace of the last
> +        # interesting thing that was declared.

This worries me a little... Do we know better before we use this?
Comment 20 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 15:33:41 PDT
Comment on attachment 629960 [details] [diff] [review]
part 9: Backend support for .ipdlh

Review of attachment 629960 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/ipdl/ipdl/lower.py
@@ +68,5 @@
>  
> +def _namespacedHeaderName(name, namespaces):
> +    pfx = '/'.join([ ns.name for ns in namespaces ])
> +    if pfx: return pfx +'/'+ name
> +    else:   return name

Nit: I think this looks a little gross but whatever.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 15:39:11 PDT
(In reply to ben turner [:bent] from comment #17)
> Comment on attachment 629958 [details] [diff] [review]
> part 7: Store gathered C++ typedefs in a set, since with headers they can
> legally be found twice
> 
> Review of attachment 629958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/cxx/ast.py
> @@ +369,5 @@
> >          self.fromtype = fromtype
> >          self.totypename = totypename
> >  
> > +    def __cmp__(self, o):
> > +        return cmp(self.totypename, o.totypename)
> 
> Hm. You could theoretically have two typedefs that are in different
> namespaces that have the same name, right? Doesn't seem like this comparison
> will take that into account.

No, that would generate an error during type checking, because we'd try to declare the same name twice in the same scope.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 15:40:28 PDT
(In reply to ben turner [:bent] from comment #18)
> Comment on attachment 629958 [details] [diff] [review]
> part 7: Store gathered C++ typedefs in a set, since with headers they can
> legally be found twice
> 
> Review of attachment 629958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/lower.py
> @@ +1261,5 @@
> > +        if not isinstance(tu, TranslationUnit) and tu not in self.visitedTus:
> > +            self.visitedTus.add(tu)
> > +            ipdl.ast.Visitor.visitTranslationUnit(self, tu)
> > +            TranslationUnit.upgrade(tu)
> > +            self.typedefs[:] = sorted(list(self.typedefSet))
> 
> Can't this just be:
> 
>   self.typedefs = sorted(self.typedefSet)
> 
> Also, there are several places below where you add to |typedefSet| without
> updating |typedefs| list. Are you guaranteed to always have a
> |visitTranslationUnit()| call later to freshen your list?
> 

No, because we hand out references to |self.typedefs| (yeah, sucks).  So when it's updated we have to ensure everyone with a reference sees the update.

> @@ +1798,5 @@
> >  
> >      def visitShmemType(self, s):
> >          if s in self.visited: return
> >          self.visited.add(s)
> > +        self.maybeTypedef('mozilla::ipc::Shmem', 'Shmem')
> 
> This change doesn't immediately look right. Did you mean to do this?

Mmm, I don't remember atm.  Will double check.
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 15:41:42 PDT
(In reply to ben turner [:bent] from comment #19)
> Comment on attachment 629959 [details] [diff] [review]
> part 8: Frontend support for .ipdlh
> 
> Review of attachment 629959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/parser.py
> @@ +216,5 @@
> > +    else:
> > +        assert tu.filetype == 'header'
> > +        # There's not really a canonical "thing" in headers.  So
> > +        # somewhat arbitrarily use the namespace of the last
> > +        # interesting thing that was declared.
> 
> This worries me a little... Do we know better before we use this?

I can't really think of anything better.  Do you have a suggestion?
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 15:44:09 PDT
(In reply to ben turner [:bent] from comment #20)
> Comment on attachment 629960 [details] [diff] [review]
> part 9: Backend support for .ipdlh
> 
> Review of attachment 629960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ipdl/ipdl/lower.py
> @@ +68,5 @@
> >  
> > +def _namespacedHeaderName(name, namespaces):
> > +    pfx = '/'.join([ ns.name for ns in namespaces ])
> > +    if pfx: return pfx +'/'+ name
> > +    else:   return name
> 
> Nit: I think this looks a little gross but whatever.

The formatting you mean?  I can change it.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 15:48:34 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #22)
> > @@ +1798,5 @@
> > >  
> > >      def visitShmemType(self, s):
> > >          if s in self.visited: return
> > >          self.visited.add(s)
> > > +        self.maybeTypedef('mozilla::ipc::Shmem', 'Shmem')
> > 
> > This change doesn't immediately look right. Did you mean to do this?
> 
> Mmm, I don't remember atm.  Will double check.

With the context it makes sense:

-        self.usingTypedefs.append(Typedef(Type('mozilla::ipc::Shmem'),
-                                          'Shmem'))
+        self.maybeTypedef('mozilla::ipc::Shmem', 'Shmem')

This is just a minor cleanup.  There wasn't any reason for that code to update |self.usingTypedefs| directly.
Comment 26 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-08 16:49:16 PDT
Comment on attachment 629958 [details] [diff] [review]
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice

Review of attachment 629958 [details] [diff] [review]:
-----------------------------------------------------------------

Ok!
Comment 28 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 17:50:49 PDT
Oh shoot ... I think I forgot to push this to try.  Friday inbound roulette!
Comment 30 Stephen Donner [:stephend] 2012-06-25 16:06:48 PDT
If QA needs to verify this bug, please set the whiteboard to [qa+], with a comment on steps to reproduce/verify; thanks!

Note You need to log in before you can comment on or make changes to this bug.