Add an alternate path determining the arguments for a function. #1508
Conversation
|
|
src/ir/function.rs
Outdated
| CXCursor_NoDeclFound => { | ||
| ty | ||
| .args() | ||
| .unwrap() |
There was a problem hiding this comment.
Hmm, unwraping here looks somewhat fishy.
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.
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.
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. |
flowbish
force-pushed
the
function_ptr_return_type
branch
from
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: |
| ) -> 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.
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.
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.
Meh, this doesn't look too bad as-is. Thanks again for looking into this!
flowbish
force-pushed
the
function_ptr_return_type
branch
from
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.
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.
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.
emilio
force-pushed
the
function_ptr_return_type
branch
from
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.