Generate better opaque blobs in the face of non-type parameters #572
Conversation
This pulls existing code out of Type's ToRustTy implementation and into an implementation of ToRustTy for TemplateInstantiation. Purely code motion.
If we hit a case where we generate an opaque blob instead of an instantiation of a generic, then we won't ever be attaching generic parameters, and can bail out of the function early.
When instantiating templates whose definitions have non-type generic parameters, prefer the layout of the instantiation type to the garbage we get from the definition's layout. In general, an instantiation's layout will always be a better choice than the definition's layout, regardless of non-type parameters. Fixes rust-lang#569
|
|
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Looks fine! What do you think about the comments?
r=me with those addressed or a reply.
src/codegen/mod.rs
Outdated
| // partial template specialization, in which case we just | ||
| // use the opaque type's layout. If we don't have a layout, | ||
| // we cross our fingers and hope for the best :-/ | ||
| debug_assert!(ctx.resolve_type_through_type_refs(decl) |
There was a problem hiding this comment.
Let's use match here instead of if let ... else return?
| } else { | ||
| inner_item.to_rust_ty(ctx) | ||
| let inner_rust_ty = inner_item.to_rust_ty(ctx); | ||
| if inner_rust_ty == aster::AstBuilder::new().ty().unit() { |
There was a problem hiding this comment.
This conditional is hacky enough it deserves at least a comment. But I really think we should just check the type to see if it's a template instantiation? Or at least, given to_rust_ty should never generate unit types unless that, what about debug_assert!ing instead?
There was a problem hiding this comment.
Will do.
I was also thinking that ToRustTy::to_rust_ty should return an Option or a Result, and then instead of ever returning () we would return None. This is a slightly more invasive change, however, and I can do it if you want, but don't feel like going out of my way at the moment :-P
It reads a little bit better this way, but is exactly equivalent.
|
Thanks for the super speedy review :) @bors-servo r=emilio |
|
📌 Commit 8b17b65 has been approved by |
Generate better opaque blobs in the face of non-type parameters When instantiating templates whose definitions have non-type generic parameters, prefer the layout of the instantiation type to the garbage we get from the definition's layout. In general, an instantiation's layout will always be a better choice than the definition's layout, regardless of non-type parameters. Fixes #569 r? @emilio
|
☀️ Test successful - status-travis |
4 participants
When instantiating templates whose definitions have non-type generic parameters, prefer the layout of the instantiation type to the garbage we get from the definition's layout. In general, an instantiation's layout will always be a better choice than the definition's layout, regardless of non-type parameters.
Fixes #569
r? @emilio