-
Notifications
You must be signed in to change notification settings - Fork 350
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
Adding &[u8] support. #117
Conversation
This change adds specifically, support for &[u8] with a corresponding rust::Slice<uint8_t> type. No other types of slice are permitted. The rationale is that it may be common to pass binary data back and forth across the FFI boundary, so it's more urgent to get this in place sooner. Broader support for other slices can wait for the future. But, both C++ and Rust-side bindings should allow the existing support to be broadened to other Slice types in future without code changes. A few specific notes: * The name "rust::Slice" might be better as "rust::SliceRef" but I'm following the precedent of "rust::Str". * It would be good to add constructors from std::span but as that's a C++20 feature, that may have to wait until C++ feature detection is resolved. * Internally, this follows the pattern of &str, where the parser will initially recognize this as Type::Ref (of Type::Slice) but then will replace that with Type::SliceRefU8. Type::Slice should not persist through later stages. As we later come to support other types of slice, we would probably want to remove Type::SliceRefU8.
Thanks -- my work codebase just needed the same thing yesterday. Will review ASAP. In the meantime, it looks like the Bazel, Buck, and Windows builds are all failing with the same error: function tests$cxxbridge02$c_try_return_sliceu8: error: undefined reference to 'tests::c_try_return_sliceu8(rust::cxxbridge02::Slice<unsigned char>) Would you be able to take a look? |
Yes, I'll take a look - but probably not for ~ 3 hours due to meetings. (My day job, sigh :) ) |
for C++ feature detection, the following should work: #ifdef __has_include
#if __has_include(<version>)
#include <version>
#endif
#endif
#if __cpp_lib_span >= 202002L
#include <span>
void has_span(std::span<char>) {}
#endif |
@programmerjake Thanks for that suggestion. There's some discussion on feature detection over here - #80 - which is why I didn't attempt to do it on this PR. |
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 is great. I am still eager to support &[T] in general but it's great to start with this, and this approach is forward compatible with supporting slices in general later, as you observed.
This change adds specifically, support for
&[u8]
with a correspondingrust::Slice<uint8_t>
type. No other types of slice are permitted. Therationale is that it may be common to pass binary data back and forth
across the FFI boundary, so it's more urgent to get this in place sooner.
Broader support for other slices can wait for the future.
But, both C++ and Rust-side bindings should allow the existing support
to be broadened to other Slice types in future without code changes.
A few specific notes:
rust::Slice
might be better asrust::SliceRef
but I'mfollowing the precedent of
rust::Str
.std::span
but as that'sa C++20 feature, that may have to wait until C++ feature detection
is resolved. Meanwhile,
rust::Slice<uint8_t>
can be constructedfrom a raw pointer and a length.
&str
, where the parser willinitially recognize this as
Type::Ref
(ofType::Slice
) but thenwill replace that with
Type::SliceRefU8
. Type::Slice should notpersist through later stages. As we later come to support other
types of slice, we would probably want to remove
Type::SliceRefU8
.