Issue 87: Bindings for Option#897
Issue 87: Bindings for Option#897kuecks wants to merge 3 commits intodtolnay:masterfrom kuecks:issue_87_option
Conversation
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
I only superficially skimmed this (and am not a reviewer here really) - mostly just wanted to say thanks for working on this! My codebase extensively uses Option<T> and this will be really nice to have.
src/cxx_optional.rs
Outdated
| //! Less used details of `CxxVector` are exposed in this module. `CxxVector` | ||
| //! itself is exposed at the crate root. |
src/cxx_optional.rs
Outdated
| /// | ||
| /// As an invariant of this API and the static analysis of the cxx::bridge | ||
| /// macro, in Rust code we can never obtain a `CxxOptional` by value. Instead in | ||
| /// Rust code we will only ever look at a vector behind a reference or smart |
|
I just want to note that this (deliberately) conflicts with #868, in which I would like to have I also ran into the issue of |
Thanks for mentioning |
|
Notes: all the changes under demo/ will be reverted, that's just my playground. The code feels quire repetitive and nasty, but I can't figure out any better way, there's just so many subtly different cases :(. Also please let me know if you want me to break it up into multiple commits somehow for reviewability, this is quite massive. |
| let element = match inner { | ||
| OptionInner::RustBox(key) => RustOption::RustBox(key.rust), | ||
| OptionInner::Ref(key) => { | ||
| if out.types.try_resolve(key.rust).is_none() { |
There was a problem hiding this comment.
This check seems really hacky :/ The intent is that if this is a builtin type like u8, the implementations are already written (and trying to implement it here would violate orphan rules anyway) so just skip it. Is there a more straightforward way to check that?
src/rust_option.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl<'a, T> RustOption<&'a T> { |
There was a problem hiding this comment.
I think some of these implementations can be removed but maybe its future proofing to have them? I can try to see which ones are unneeded
src/rust_option.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl RustOption<crate::private::RustStr> { |
There was a problem hiding this comment.
These seemed particularly sketchy
syntax/resolve.rs
Outdated
| match self.try_resolve(ident) { | ||
| Some(resolution) => resolution, | ||
| None => panic!("Unable to resolve type `{}`", ident), | ||
| None => { |
There was a problem hiding this comment.
hrmm i think this should be reverted
tests/ffi/lib.rs
Outdated
| fn c_return_ref_optional(i: &u8) -> &CxxOptional<u8>; | ||
| fn c_return_mut_optional(i: &mut u8) -> &mut CxxOptional<u8>; | ||
| fn c_return_rust_option_box() -> Option<Box<Shared>>; | ||
| fn c_return_rust_ref_option_shared() -> Option<&'static Shared>; |
There was a problem hiding this comment.
Refs were by far the most complicated one so I tried to have extensive testing (which caught a lot of bugs). Are there any cases I missed?
|
Is this pr going anywhere? I am quite interested in this feature. |
|
@djpiper28 yes I am still interested in pushing this through, I think dtolnay is (understandably) just quite busy unfortunately. I may explore alternative implementations, specifically reference_wrapper, in the meantime |
|
Any progress? This feature would be pretty big... what's blocking it? |
|
is anyone doing this? |
6 participants
Added bindings to rust's std::option::Option, and C++'s std::optional::optional. Still a WIP but this is able to compile and run a simple example