-
Notifications
You must be signed in to change notification settings - Fork 328
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
The ability to generate trait methods using types #1054
base: master
Are you sure you want to change the base?
Conversation
src/bindgen/parser.rs
Outdated
if matches!(item, syn::ImplItem::Const(_)) { | ||
impls_with_assoc_consts.push(item_impl); | ||
} else if let syn::ImplItem::Type(t) = item { | ||
impls_with_assoc_ty.push((&t.ident, &t.ty)); |
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.
This array seems wrong, it's not storing impls
, but ident and types, so maybe just call the vector associated_tys
or so?
src/bindgen/parser.rs
Outdated
Err(..) => continue, | ||
}; | ||
|
||
for (ident, ty) in &impls_with_assoc_ty { |
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.
So I'm a bit confused as to what are you trying to do here. Can you add some comments to clarify?
Let's see if I get this right... When you see a method, you go through the return and argument types, and try to fix them up by matching with the associated types.
I don't think this is quite the right thing to do. I guess this does work for very simple cases, but:
- At the very least you should do this recursively.
- I think ideally, we emit the right thing which would be something like:
typedef void Dummy0_DummyTrait_DummyIn;
// etc
And then load_syn_method
just knows about associated paths with Self
and deals with that. Doing this mapping on the syn types seems a bit funky / sketchy at least.
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.
@emilio in my first trial version, the implementation was approximately the way you described, but I thought that it would be excessive. If you think that it will be better - I will do so
Perhaps this execution will be acceptable... |
When writing similar code now:
Incorrect code will be generated, looking something like this:
My fix allows the code to be generated correctly: