Replace clang.rs iterator with generic boxed Map<Range<>> #213
Conversation
|
|
|
This is a simpler & cleaner alternative to PR #189 |
|
I'm still not totally sure I like this more than the status quo or #189. In general, I think the common things that have these iterators are:
I think #189 cleaned up would end up being simpler and more idiomatic than this PR, but I also don't dislike this... Opinions, @fitzgen? |
|
I prefer #189 -- we shouldn't need to box here. |
|
Thank you for your feedback. I agree, we should not need to box here. My understanding is there is little performance gain to be have considering it redirect to ffi calls. |
|
I'll re-open and clean up PR #189 |
We have unboxed closures in Rust already, I think the "return impl trait" (or |
|
Yep, in general you want something like |
|
☔ The latest upstream changes (presumably #204) made this pull request unmergeable. Please resolve the merge conflicts. |
Add functions: -ffi_call_index_iterator -> Box<ExactSizeIterator> for foreign function with unsigned length and index -ffi_call_index_iterator_check_positive -> Option<Box<ExactSizeIterator>> for foreign function with signed length and index that can be -1 These function take two closures as parameters. This interface should guide the correct usage of having thin closures arround the actual two relevant ffi functions. The ad hoc iterator where basically implementing (0..clang_getNum()).map( |id| clang_getItem( self.x, id ) ) with some additional complexity if clang_getNum() could be -1. Using these new function greatly improve maintainability since now everything is in the function getting a specific iterator, and all iterators implement ExactSizeIterator consistently. The trade off is we now have dynamic dispatch, but that should not be too problematic since we are calling a foreign function to get the item. Eventually rust will support impl trait (experimental feature conservative_impl_trait which has a very close syntax) at which point the Box can be replaced if needed and appropriate.
jeanphilippeD
force-pushed
the
ffi_indexable
branch
from
bd715b5
to
833f2a3
Compare
|
Hi @fitzgen , Thank you for your comment. The commit 0c769db shows the migration path. impl trait was implemented in rust 1.12, but will take some time before it is stable. This version seems to be dealing with lifetime in a better way than what we have in master, it seems before we were copying CXComment and CXType, and the iterator may have outlived the parent Comment and Type (is that an issue here?). I still feel it make sense to use standard iterator types but if there are more important things to consider, I'll improve PR #189 Thanks, |
|
☔ The latest upstream changes (presumably #333) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I believe this can be closed for now, due to the lack of activity. Thanks for the PR and the discussion! |
Add functions:
-ffi_call_index_iterator -> Box
for foreign function with unsigned length and index
-ffi_call_index_iterator_check_positive -> Option<Box>
for foreign function with signed length and index that can be -1
These function take two closures as parameters.
This interface should guide the correct usage of having thin
closures arround the actual two relevant ffi functions.
The ad hoc iterator where basically implementing
(0..clang_getNum()).map( |id| clang_getItem( self.x, id ) )
with some additional complexity if clang_getNum() could be -1.
Using these new function greatly improve maintainability
since now everything is in the function getting a specific iterator,
and all iterators implement ExactSizeIterator consistently.
The trade off is we now have dynamic dispatch, but that should not
be too problematic since we are calling a foreign function to get
the item.
Eventually rust will support impl trait (experimental feature
conservative_impl_trait which has a very close syntax) at which
point the Box can be replaced if needed and appropriate.