-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Do not rewrite ADTs mentioned in extern blocks #960
Conversation
616cd8d
to
97bbed5
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
Co-authored-by: Khyber Sen <[email protected]>
This reverts commit 130bda9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
29fa269
to
9d20b20
Compare
9d20b20
to
0a4557d
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, that should be covered by .walk
instead, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.