Closed
Bug 521898
Opened 15 years ago
Closed 12 years ago
IPDL should allow me to use a union in another protocol
Categories
(Core :: IPC, defect)
Core
IPC
Tracking
()
RESOLVED
FIXED
mozilla16
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.
Assignee | ||
Comment 2•12 years ago
|
||
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
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Assignee | ||
Comment 4•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [b2g:blocking+][soft]
Updated•12 years ago
|
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Whiteboard: [b2g:blocking+][soft] → [soft]
Assignee | ||
Comment 5•12 years ago
|
||
Done, working. Splitting for review next time I get a bit of coding time (hopefully tomorrow).
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #629944 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
Attachment #629949 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #629948 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #629950 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #629955 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #629958 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #629959 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #629960 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #629961 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 16•12 years ago
|
||
Attachment #629962 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•12 years ago
|
Attachment #629954 -
Flags: review?(bent.mozilla)
Reporter | ||
Updated•12 years ago
|
Attachment #629944 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629948 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629949 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629950 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629954 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629955 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Comment 17•12 years ago
|
||
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.
Reporter | ||
Comment 18•12 years ago
|
||
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?
Reporter | ||
Comment 19•12 years ago
|
||
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+
Reporter | ||
Comment 20•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #629961 -
Flags: review?(bent.mozilla) → review+
Reporter | ||
Updated•12 years ago
|
Attachment #629962 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Assignee | ||
Comment 22•12 years ago
|
||
(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.
Assignee | ||
Comment 23•12 years ago
|
||
(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?
Assignee | ||
Comment 24•12 years ago
|
||
(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.
Assignee | ||
Comment 25•12 years ago
|
||
(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.
Reporter | ||
Comment 26•12 years ago
|
||
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+
Assignee | ||
Comment 27•12 years ago
|
||
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
Assignee | ||
Comment 28•12 years ago
|
||
Oh shoot ... I think I forgot to push this to try. Friday inbound roulette!
Comment 29•12 years ago
|
||
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
Closed: 12 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•12 years ago
|
Whiteboard: [soft][qa?] → [soft][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•