IPDL should allow me to use a union in another protocol

RESOLVED FIXED in mozilla16

Status

()

Core
IPC
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: cjones)

Tracking

Trunk
mozilla16
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:+)

Details

(Whiteboard: [soft][qa-])

Attachments

(11 attachments)

8.03 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
2.54 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
9.52 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
3.43 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
13.35 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
2.72 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
6.12 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
13.71 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
4.32 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
1.93 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: review+
Details | Diff | Splinter Review
5.69 KB, patch
Ben Turner (not reading bugmail, use the needinfo flag!)
: 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.

Comment 1

6 years ago
Ben,

Did you ever get to work around this one?
Blocks: 666693

Updated

5 years ago
No longer blocks: 666693

Updated

5 years ago
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).
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
Attachment #629944 - Flags: review?(bent.mozilla)
Created attachment 629948 [details] [diff] [review]
part 2: Rename StructField .type to .typespec to avoid unintended "shadowing"
Created attachment 629949 [details] [diff] [review]
part 3: Rename ProtocolInclude to Include in preparation of more general usage
Attachment #629949 - Flags: review?(bent.mozilla)
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
Attachment #629948 - Flags: review?(bent.mozilla)
Attachment #629950 - Flags: review?(bent.mozilla)
Created attachment 629954 [details] [diff] [review]
part 5: Prepare for TU .protocol possibly being null (for headers)
Created attachment 629955 [details] [diff] [review]
part 6: Refactor the way structs/unions are declared
Attachment #629955 - Flags: review?(bent.mozilla)
Created attachment 629958 [details] [diff] [review]
part 7: Store gathered C++ typedefs in a set, since with headers they can legally be found twice
Attachment #629958 - Flags: review?(bent.mozilla)
Created attachment 629959 [details] [diff] [review]
part 8: Frontend support for .ipdlh
Attachment #629959 - Flags: review?(bent.mozilla)
Created attachment 629960 [details] [diff] [review]
part 9: Backend support for .ipdlh
Attachment #629960 - Flags: review?(bent.mozilla)
Created attachment 629961 [details] [diff] [review]
part 10: Add .ipdlh to the test harness and fix an annoying build warning
Attachment #629961 - Flags: review?(bent.mozilla)
Created attachment 629962 [details] [diff] [review]
Tests for bug 521898
Attachment #629962 - 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6beff74a76e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/038ac44e564b
https://hg.mozilla.org/integration/mozilla-inbound/rev/b96cb0e882eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/f713eb64e8e1
https://hg.mozilla.org/integration/mozilla-inbound/rev/fece9a6bae3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a3bbdcfe04
https://hg.mozilla.org/integration/mozilla-inbound/rev/7befc650f055
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4f5ef66e6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/02d6424b10b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b10a0c183f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/db86ccc22a84
Oh shoot ... I think I forgot to push this to try.  Friday inbound roulette!
Looks like you won this time :-)

https://hg.mozilla.org/mozilla-central/rev/6beff74a76e6
https://hg.mozilla.org/mozilla-central/rev/038ac44e564b
https://hg.mozilla.org/mozilla-central/rev/b96cb0e882eb
https://hg.mozilla.org/mozilla-central/rev/f713eb64e8e1
https://hg.mozilla.org/mozilla-central/rev/fece9a6bae3f
https://hg.mozilla.org/mozilla-central/rev/54a3bbdcfe04
https://hg.mozilla.org/mozilla-central/rev/7befc650f055
https://hg.mozilla.org/mozilla-central/rev/1b4f5ef66e6e
https://hg.mozilla.org/mozilla-central/rev/02d6424b10b8
https://hg.mozilla.org/mozilla-central/rev/8b10a0c183f0
https://hg.mozilla.org/mozilla-central/rev/db86ccc22a84

Per the tree rules, please set the target milestone when landing on inbound.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
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?]

Updated

5 years ago
Whiteboard: [soft][qa?] → [soft][qa-]
You need to log in before you can comment on or make changes to this bug.