Skip to content
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 API to get slice from CxxVector #321

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions gen/src/write.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,13 @@ fn write_cxx_vector(out: &mut OutFile, vector_ty: &Type, element: &Ident, types:
);
writeln!(out, " return s.size();");
writeln!(out, "}}");
writeln!(
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering whether I can or should generate src/cxx.h|cc from this. Need to look into that...

out,
"const {} *cxxbridge04$std$vector${}$data(const ::std::vector<{}> &s) noexcept {{",
inner, instance, inner,
);
writeln!(out, " return s.data();");
writeln!(out, "}}");
writeln!(
out,
"const {} *cxxbridge04$std$vector${}$get_unchecked(const ::std::vector<{}> &s, size_t pos) noexcept {{",
Expand Down
25 changes: 25 additions & 0 deletions macro/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,7 @@ fn expand_cxx_vector(namespace: &Namespace, elem: &Ident) -> TokenStream {
let name = elem.to_string();
let prefix = format!("cxxbridge04$std$vector${}{}$", namespace, elem);
let link_size = format!("{}size", prefix);
let link_data = format!("{}data", prefix);
let link_get_unchecked = format!("{}get_unchecked", prefix);
let unique_ptr_prefix = format!("cxxbridge04$unique_ptr$std$vector${}{}$", namespace, elem);
let link_unique_ptr_null = format!("{}null", unique_ptr_prefix);
Expand All @@ -859,6 +860,30 @@ fn expand_cxx_vector(namespace: &Namespace, elem: &Ident) -> TokenStream {
}
unsafe { __vector_size(v) }
}
fn __vector_slice(v: &::cxx::CxxVector<Self>) -> &[Self] {
extern "C" {
#[link_name = #link_size]
fn __vector_size(_: &::cxx::CxxVector<#elem>) -> usize;
#[link_name = #link_data]
fn __vector_data(_: &::cxx::CxxVector<#elem>) -> *const #elem;
}
let data = unsafe { __vector_data(v) };
if (data.is_null())
{
// Avoid undefined behaviour in slice::from_raw_parts() as
// std::vector::data() returns nullptr if no capacity().
// Alternative: Use from_raw_parts(NonNull::dangling(), 0)
let ret = <&[Self]>::default(); // empty slice
ret
}
else
{
// std::vector with capacity() has valid data ptr, even if
// size() == 0.
let ret = unsafe { std::slice::from_raw_parts(data, __vector_size(v)) };
ret
}
}
unsafe fn __get_unchecked(v: &::cxx::CxxVector<Self>, pos: usize) -> &Self {
extern "C" {
#[link_name = #link_get_unchecked]
Expand Down
4 changes: 4 additions & 0 deletions src/cxx.cc
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ void cxxbridge04$unique_ptr$std$string$drop(
const std::vector<CXX_TYPE> &s) noexcept { \
return s.size(); \
} \
const CXX_TYPE* cxxbridge04$std$vector$##RUST_TYPE##$data( \
const std::vector<CXX_TYPE> &s) noexcept { \
return s.data(); \
} \
const CXX_TYPE *cxxbridge04$std$vector$##RUST_TYPE##$get_unchecked( \
const std::vector<CXX_TYPE> &s, size_t pos) noexcept { \
return &s[pos]; \
Expand Down
40 changes: 40 additions & 0 deletions src/cxx_vector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ use core::fmt::{self, Display};
use core::marker::PhantomData;
use core::mem;
use core::ptr;
use std::slice;


/// Binding to C++ `std::vector<T, std::allocator<T>>`.
///
Expand Down Expand Up @@ -63,6 +65,15 @@ where
pub unsafe fn get_unchecked(&self, pos: usize) -> &T {
T::__get_unchecked(self, pos)
}

/// Returns a slice to the underlying vector data
///
/// An alternative approach would be to base your FFI API directly on a
/// rust::Slice<T> instead of a std::vector<T>.
pub fn get_slice(&self) -> &[T] {
let slice = T::__vector_slice(self);
slice
}
}

pub struct Iter<'a, T> {
Expand Down Expand Up @@ -122,6 +133,7 @@ where
pub unsafe trait VectorElement: Sized {
const __NAME: &'static dyn Display;
fn __vector_size(v: &CxxVector<Self>) -> usize;
fn __vector_slice(v: &CxxVector<Self>) -> &[Self];
unsafe fn __get_unchecked(v: &CxxVector<Self>, pos: usize) -> &Self;
fn __unique_ptr_null() -> *mut c_void;
unsafe fn __unique_ptr_raw(raw: *mut CxxVector<Self>) -> *mut c_void;
Expand All @@ -145,6 +157,34 @@ macro_rules! impl_vector_element {
}
unsafe { __vector_size(v) }
}
fn __vector_slice(v: &CxxVector<$ty>) -> &[$ty] {
extern "C" {
attr! {
#[link_name = concat!("cxxbridge04$std$vector$", $segment, "$data")]
fn __vector_data(_: &CxxVector<$ty>) -> &$ty;
}
attr! {
#[link_name = concat!("cxxbridge04$std$vector$", $segment, "$size")]
fn __vector_size(_: &CxxVector<$ty>) -> usize;
}
}
let data : *const $ty = unsafe { __vector_data(v) };
if (data.is_null())
{
// Avoid undefined behaviour in slice::from_raw_parts() as
// std::vector::data() returns nullptr if no capacity().
// Alternative: Use from_raw_parts(NonNull::dangling(), 0)
let ret = <&[$ty]>::default(); // empty slice
ret
}
else
{
// std::vector with capacity() has valid data ptr, even if
// size() == 0.
let ret = unsafe { slice::from_raw_parts(data, __vector_size(v)) };
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still undefined behavior, I think. As far as I can tell nothing in https://en.cppreference.com/w/cpp/container/vector/data would guarantee that data produces a sufficiently aligned pointer in the empty case. Can you find that in the standard, or implement a different way?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. Went after the implementation behaviour that always has been instead of reading the standard.

Fix would be to check the size() == 0 instead the ptr, which I see you have done in your implementation. Thanks for fixing my code up. ;)

ret
}
}
unsafe fn __get_unchecked(v: &CxxVector<$ty>, pos: usize) -> &$ty {
extern "C" {
attr! {
Expand Down
15 changes: 15 additions & 0 deletions tests/ffi/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ pub mod ffi {
fn r_take_ref_rust_vec_string(v: &Vec<String>);
fn r_take_enum(e: Enum);

fn r_access_vector_u8_as_slice(v: &CxxVector<u8>);
fn r_access_vector_u8_as_slice_empty(v: &CxxVector<u8>);

fn r_try_return_void() -> Result<()>;
fn r_try_return_primitive() -> Result<usize>;
fn r_try_return_box() -> Result<Box<R>>;
Expand All @@ -165,6 +168,8 @@ pub type R = usize;

pub struct R2(usize);

use cxx::CxxVector;

impl R2 {
fn get(&self) -> usize {
self.0
Expand Down Expand Up @@ -327,6 +332,16 @@ fn r_take_enum(e: ffi::Enum) {
let _ = e;
}

fn r_access_vector_u8_as_slice(v: &CxxVector<u8>) {
let s = v.get_slice();
assert_eq!(s, [86, 75, 30, 9]);
}

fn r_access_vector_u8_as_slice_empty(v: &CxxVector<u8>) {
let s = v.get_slice();
assert!(s.is_empty());
}

fn r_try_return_void() -> Result<(), Error> {
Ok(())
}
Expand Down
23 changes: 23 additions & 0 deletions tests/ffi/tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,29 @@ extern "C" const char *cxx_run_test() noexcept {
std::unique_ptr<std::string>(new std::string("2020")));
r_take_enum(Enum::AVal);

// Caller owns data handed into rust APIs
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for that did not really fit into the "take" or "return" tests.

I initially had a c_take...() test that would create a std::vector and then call into the r_access_...() test function, but I found that overly complex as the c_take...() test did not add anything. Hence, directly testing the API in r_access..() here.

{
std::vector<std::uint8_t> v{86, 75, 30, 9};
r_access_vector_u8_as_slice(v);
}
{
// Empty std::vector has data() == nullptr.
// This shall produce an empty slice.
std::vector<std::uint8_t> v;
ASSERT(v.data() == nullptr);
ASSERT(v.size() == 0);
r_access_vector_u8_as_slice_empty(v);
}
{
// Empty std::vector with capacity has data() != nullptr and size() == 0.
// This shall produce an empty slice.
std::vector<std::uint8_t> v;
v.reserve(10);
ASSERT(v.data() != nullptr);
ASSERT(v.size() == 0);
r_access_vector_u8_as_slice_empty(v);
}

ASSERT(r_try_return_primitive() == 2020);
try {
r_fail_return_primitive();
Expand Down