Closed Bug 521898 Opened 10 years ago Closed 7 years ago

IPDL should allow me to use a union in another protocol

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla16
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: bent.mozilla, Assigned: cjones)

References

Details

(Whiteboard: [soft][qa-])

Attachments

(11 files)

8.03 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
2.54 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
9.52 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
3.43 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
13.35 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
2.72 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
6.12 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
13.71 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.32 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
1.93 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
5.69 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
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.
Ben,

Did you ever get to work around this one?
No longer blocks: 666693
Blocks: 666693
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
(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.
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.
Whiteboard: [b2g:blocking+][soft]
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+][soft] → [soft]
Done, working.  Splitting for review next time I get a bit of coding time (hopefully tomorrow).
Attachment #629948 - Flags: review?(bent.mozilla)
Attachment #629950 - Flags: review?(bent.mozilla)
Attachment #629954 - Flags: review?(bent.mozilla)
Attachment #629944 - Flags: review?(bent.mozilla) → review+
Attachment #629948 - Flags: review?(bent.mozilla) → review+
Attachment #629949 - Flags: review?(bent.mozilla) → review+
Attachment #629950 - Flags: review?(bent.mozilla) → review+
Attachment #629954 - Flags: review?(bent.mozilla) → review+
Attachment #629955 - Flags: review?(bent.mozilla) → review+
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 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 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?
Attachment #629959 - Flags: review?(bent.mozilla) → review+
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.
Attachment #629960 - Flags: review?(bent.mozilla) → review+
Attachment #629961 - Flags: review?(bent.mozilla) → review+
Attachment #629962 - Flags: review?(bent.mozilla) → review+
(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.
(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.
(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?
(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.
(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 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!
Attachment #629958 - Flags: review?(bent.mozilla) → review+
Oh shoot ... I think I forgot to push this to try.  Friday inbound roulette!
If QA needs to verify this bug, please set the whiteboard to [qa+], with a comment on steps to reproduce/verify; thanks!
Whiteboard: [soft] → [soft][qa?]
Whiteboard: [soft][qa?] → [soft][qa-]
You need to log in before you can comment on or make changes to this bug.