-
Notifications
You must be signed in to change notification settings - Fork 685
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
Add an alternate path determining the arguments for a function. #1508
Conversation
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.
Thanks for looking at this! Can you describe what the objc failures are?
src/ir/function.rs
Outdated
CXCursor_NoDeclFound => { | ||
ty | ||
.args() | ||
.unwrap() |
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.
Hmm, unwrap
ing here looks somewhat fishy.
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.
Yes, very much.
src/ir/function.rs
Outdated
|
||
let ret = Item::from_ty_or_ref( | ||
ty_ret_type, | ||
ty_ret_type.declaration(), |
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.
Hmm, so I see how this works, but it is a bit fishy to pass the declaration here just to match it from NoDeclFound afterwards. I suspect this can break other stuff but I'd need to think about what...
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.
You're right about that, I think that this is the source of the test failures below.
Thanks for taking a look at this. I would love some help restructuring this and getting this PR into a good state. It would be great if you could take a look at the test failures, as I don't really understand what's going on there. |
2894f7d
to
c0957c7
Compare
I updated the PR with another shot at fixing this – I feel a bit more confident in the code changes this time. As a side effect, it seems to have fixed a previously-broken test: |
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.
Thanks, this change makes much more sense to me! Looks good, just some nits / suggestions, if you want. Looks good otherwise!
) -> Vec<(Option<String>, TypeId)> { | ||
match (cursor.args(), ty.args()) { | ||
(Some(cursor_args), Some(ty_args)) => { | ||
ty_args.iter().enumerate().map(|(i, 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, what about the following instead, which could be much shorter:
let args_for_names = cursor.args().unwrap();
let ty_args = ty.args();
let ty_args = ty_args.as_ref().unwrap_or(&args_for_names);
// Write the loop only once.
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.
I tried to rewrite this a few different ways but found everything unsatisfactory. Cursor::args() and Type::args() return different types and I haven't figured out a good way in Rust to work with iterators of different concrete types well. Definitely open to suggestions!
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.
Meh, this doesn't look too bad as-is. Thanks again for looking into this!
c0957c7
to
7eee1df
Compare
src/ir/function.rs
Outdated
@@ -353,28 +378,14 @@ impl FunctionSig { | |||
ty.declaration() | |||
}; | |||
|
|||
let mut args: Vec<_> = match cursor.kind() { | |||
let mut args = match cursor.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.
I think this needs a small rebase (using kind
instead of cursor.kind()
).
) -> Vec<(Option<String>, TypeId)> { | ||
match (cursor.args(), ty.args()) { | ||
(Some(cursor_args), Some(ty_args)) => { | ||
ty_args.iter().enumerate().map(|(i, 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.
Meh, this doesn't look too bad as-is. Thanks again for looking into this!
…signatures. This fixes the issue seen in rust-lang#1500 where function pointers as a return type have their parameters incorrectly generated. This also fixes a broken test case, objc_property_fnptr, as its types are now generated correctly.
e01a6bc
to
1092778
Compare
Just did the rebase myself. Thanks! |
This fixes the issue seen in #1500 where function pointers as a return type have their parameters incorrectly generated.
This issue was happening because the cursor was being used to determine the arguments for a function prototype. This is fine, but function prototypes in the return position have no cursor associated with them. This would cause the code to reuse the cursor from the function returning it and copy its parameters. Thus
int (*func(void))(int, int)
would be parsed asint (*func(void))(void)
.This causes issues with some Objective C tests for reasons I don't quite understand, so it's not yet ready. I'd love some input on this, or maybe even for someone to adopt this patch if they really feel so motivated.