Trace methods in CompInfo's TypeCollector impl #411
Conversation
|
|
|
Added two more commits cleaning up more of the type tracing. The only TODO left about type tracing is for |
|
It isn't clear to me what needs to happen with regards to vtables here. It seems like they should already be handled via the base's tracing? |
| types.insert(repr); | ||
| } | ||
| } | ||
| TypeKind::UnresolvedTypeRef(_, _, Some(id)) => { |
There was a problem hiding this comment.
We could just mark UnresolvedTypeRef as unreachable I think.
There was a problem hiding this comment.
Yeah, I wasn't thinking... That invariant should definitely hold here.
| // Module -> children edges are "weak", and we do not want to | ||
| // trace them. If we did, then whitelisting wouldn't work as | ||
| // expected: everything in every module would end up | ||
| // whitelisted. |
There was a problem hiding this comment.
This is only true because we start tracing through the root module right? we should be able of making that exception? Anyway it's fine for now.
There was a problem hiding this comment.
Its because we trace the parent_id to find the containing module, which will then end up tracing everything in the module, and the module's parent module which will end up tracing everything in that module, etc etc.
There was a problem hiding this comment.
And also because of tracing the root module too, I suppose.
| types.insert(method.signature); | ||
| } | ||
|
|
||
| // FIXME(emilio): VTable? |
There was a problem hiding this comment.
Yeah, I think every type that could potentially be used in the vtable would be caught by the bases or the methods, so you should be able to remove this comment.
|
☔ The latest upstream changes (presumably #414) made this pull request unmergeable. Please resolve the merge conflicts. |
We trace all things in the vtable via tracing the base types.
fitzgen
force-pushed
the
js-value-and-js-why-magic
branch
from
024b79c to
93dd68f
Compare
|
@bors-servo r=emilio |
|
📌 Commit 93dd68f has been approved by |
|
☀️ Test successful - status-travis |
Fixes #410
r? @emilio