Do not rewrite ADTs mentioned in extern blocks #960
Conversation
oinoom
force-pushed
the
fix.rewriter.foreign
branch
from
616cd8d to
97bbed5
Compare
oinoom
force-pushed
the
fix.rewriter.foreign
branch
from
97bbed5 to
b90b646
Compare
|
Also, I changed "addresses" to "fixes" in the PR description as I don't think "addresses" is one of the keywords for closing an issue from a PR. |
spernsteiner
left a comment
spernsteiner
left a comment
There was a problem hiding this comment.
Looks like a good approach overall
|
@spernsteiner I’m not clear on what further changes you’d like to see. Could you let me know if I’ve missed any concerns of yours? |
This reverts commit 130bda9.
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Just one small nit left: walk_adts instead of walk_adt, as the latter sounds like it's walking over only one. Otherwise, it all LGTM now!
kkysen
changed the title
oinoom
force-pushed
the
fix.rewriter.foreign
branch
from
29fa269 to
9d20b20
Compare
oinoom
force-pushed
the
fix.rewriter.foreign
branch
from
9d20b20 to
0a4557d
Compare
oinoom
force-pushed
the
fix.rewriter.foreign
branch
from
46b5ca8 to
b854368
Compare
| { | ||
| for arg in ty.walk() { | ||
| if let GenericArgKind::Type(ty) = arg.unpack() { | ||
| if let TyKind::Adt(adt_def, _) = ty.kind() { |
There was a problem hiding this comment.
Why don't we need to substitute generics here? Then generic fields will just be TyKind::Param and we won't recognize that it may be an ADT or other type we need to recurse into.
There was a problem hiding this comment.
Oh wait, that should be covered by .walk instead, right?
There was a problem hiding this comment.
We'll indeed see TyKind::Param when visiting fields of generic structs, but walk will separately visit the type that would have been substituted in.
struct Wrapper<T> { x: T }
struct Inner { y: u32 }
extern "C" {
fn foo(ptr: *mut Wrapper<Inner>);
}walk(*mut Wrapper<Inner>) will visit Wrapper<Inner> and Inner, so it's okay that the traversal of the x field sees only TyKind::Param instead of Inner.
Fixes #948. Fixes an issue where we would rewrite structs and their fields even though they appear in extern blocks.