has vtable analysis #850
has vtable analysis #850
Conversation
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @fitzgen (or someone else) soon. |
src/ir/analysis/has_vtable.rs
Outdated
| }; | ||
|
|
||
| // Migrate this comment from the original has_vtable in ty.rs | ||
| // FIXME: Can we do something about template parameters? Huh... |
There was a problem hiding this comment.
Let's replace this comment with:
// TODO #851: figure out a way to handle deriving from template type parameters.
src/ir/derive.rs
Outdated
| fn can_derive_default(&self, | ||
| ctx: &BindgenContext, | ||
| fn can_derive_default(&'a self, | ||
| ctx: &'a BindgenContext, |
There was a problem hiding this comment.
These references shouldn't be 'a; that should just be for Self::Extra's use. Unless they're required and I'm missing why...
src/ir/comp.rs
Outdated
| /// Do we see a virtual function during parsing? | ||
| /// Get the has_vtable boolean. | ||
| pub fn get_has_vtable(&self) -> bool { | ||
| return self.has_vtable; |
There was a problem hiding this comment.
Let's rename these to fn has_own_virtual_method and self.has_own_virtual_method to make it more clear that we are not walking the inheritance DAG.
src/ir/ty.rs
Outdated
|
|
||
| fn has_vtable(&self, ctx: &BindgenContext, itemid: &ItemId) -> bool { | ||
| ctx.lookup_item_id_has_vtable(itemid) | ||
| } |
There was a problem hiding this comment.
I'm on the edge... do you think this trait and these impls are carrying their weight? I guess I like the Item and ItemId impls, but this one seems pretty shaky to me...
There was a problem hiding this comment.
I think the ItemId and Item one doesn't get used. The has_vtable is called on Ty in the codegen/mod.rs inside the impl CodeGenerator for CompInfo. needs_explicit_vtable in CompInfo and is_unsized in Ty and CompInfo also call the has_vtable. I think for Ty and CompInfo, calling lookup_item_id_has_vtable directly might be better. But if we want to keep the same interface for all the types, I'm also fine with that
There was a problem hiding this comment.
Right -- anyone calling this impl could (doesn't mean that they are now, but they could) just call the Item or ItemId impl and it would be cleaner than the hoops being jumped through here.
Yeah, I've convinced myself: let's remove the HasVtable impls for everything except Item and ItemId.
| Self::consider_edge(edge_kind) { | ||
| dependencies.entry(sub_item) | ||
| .or_insert(vec![]) | ||
| .push(item); |
There was a problem hiding this comment.
Want to do a follow up PR to pull the creation of the dependencies graph out and share its definition between all of our analyses? This is getting copied with slight changes a little too much now, and its clear we need a helper function.
|
@bors-servo r+ |
|
@bors r+ |
|
Ok, manually merging for now. |
Fix #765 r? @fitzgen