Skip to content

Commit

Permalink
Migrate Repeated::{push, set} and Map::insert to use the IntoProxied …
Browse files Browse the repository at this point in the history 
…trait.

  * The public Repeated::{push, set} and Map::insert methods now accept any value that implements IntoProxied<T>, allowing us to move owned values instead of copying them.
  * This change also updates the FFI layer for strings/bytes in the repeated and maps thunks to accept a std::string* that can be moved rather than a PtrAndLen type that needs to be copied.
  * Tests are updated to no longer .as_view() when setting a message / string on a repeated / map field. The IntoProxied trait makes calling .as_view() obsolete.

PiperOrigin-RevId: 648657921
  • Loading branch information
@buchgr @copybara-github
buchgr authored and copybara-github committed Jul 4, 2024
1 parent e931905 commit bd2fe9c
Show file tree
Hide file tree
Showing 15 changed files with 360 additions and 322 deletions.
77 changes: 51 additions & 26 deletions rust/cpp.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

use crate::__internal::{Enum, Private};
use crate::{
Map, MapIter, Mut, ProtoBytes, ProtoStr, ProtoString, Proxied, ProxiedInMapValue,
IntoProxied, Map, MapIter, Mut, ProtoBytes, ProtoStr, ProtoString, Proxied, ProxiedInMapValue,
ProxiedInRepeated, Repeated, RepeatedMut, RepeatedView, View,
};
use core::fmt::Debug;
Expand All @@ -18,7 +18,7 @@ use std::convert::identity;
use std::ffi::{c_int, c_void};
use std::fmt;
use std::marker::PhantomData;
use std::mem::MaybeUninit;
use std::mem::{ManuallyDrop, MaybeUninit};
use std::ops::Deref;
use std::ptr::{self, NonNull};
use std::slice;
Expand Down Expand Up @@ -393,20 +393,27 @@ impl<'msg> InnerRepeatedMut<'msg> {
}

trait CppTypeConversions: Proxied {
type InsertElemType;
type ElemType;

fn elem_to_view<'msg>(v: Self::ElemType) -> View<'msg, Self>;
fn into_insertelem(v: Self) -> Self::InsertElemType;
}

macro_rules! impl_cpp_type_conversions_for_scalars {
($($t:ty),* $(,)?) => {
$(
impl CppTypeConversions for $t {
type InsertElemType = Self;
type ElemType = Self;

fn elem_to_view<'msg>(v: Self) -> View<'msg, Self> {
v
}

fn into_insertelem(v: Self) -> Self {
v
}
}
)*
}
Expand All @@ -415,19 +422,31 @@ macro_rules! impl_cpp_type_conversions_for_scalars {
impl_cpp_type_conversions_for_scalars!(i32, u32, i64, u64, f32, f64, bool);

impl CppTypeConversions for ProtoString {
type InsertElemType = CppStdString;
type ElemType = PtrAndLen;

fn elem_to_view<'msg>(v: PtrAndLen) -> View<'msg, ProtoString> {
ptrlen_to_str(v)
}

fn into_insertelem(v: Self) -> CppStdString {
let v = ManuallyDrop::new(v);
v.inner.owned_ptr
}
}

impl CppTypeConversions for ProtoBytes {
type InsertElemType = CppStdString;
type ElemType = PtrAndLen;

fn elem_to_view<'msg>(v: Self::ElemType) -> View<'msg, Self> {
ptrlen_to_bytes(v)
}

fn into_insertelem(v: Self) -> CppStdString {
let v = ManuallyDrop::new(v);
v.inner.owned_ptr
}
}

macro_rules! impl_repeated_primitives {
Expand All @@ -446,15 +465,15 @@ macro_rules! impl_repeated_primitives {
extern "C" {
fn $new_thunk() -> RawRepeatedField;
fn $free_thunk(f: RawRepeatedField);
fn $add_thunk(f: RawRepeatedField, v: <$t as CppTypeConversions>::ElemType);
fn $add_thunk(f: RawRepeatedField, v: <$t as CppTypeConversions>::InsertElemType);
fn $size_thunk(f: RawRepeatedField) -> usize;
fn $get_thunk(
f: RawRepeatedField,
i: usize) -> <$t as CppTypeConversions>::ElemType;
fn $set_thunk(
f: RawRepeatedField,
i: usize,
v: <$t as CppTypeConversions>::ElemType);
v: <$t as CppTypeConversions>::InsertElemType);
fn $clear_thunk(f: RawRepeatedField);
fn $copy_from_thunk(src: RawRepeatedField, dst: RawRepeatedField);
fn $reserve_thunk(
Expand All @@ -480,8 +499,8 @@ macro_rules! impl_repeated_primitives {
unsafe { $size_thunk(f.as_raw(Private)) }
}
#[inline]
fn repeated_push(mut f: Mut<Repeated<$t>>, v: View<$t>) {
unsafe { $add_thunk(f.as_raw(Private), v.into()) }
fn repeated_push(mut f: Mut<Repeated<$t>>, v: impl IntoProxied<$t>) {
unsafe { $add_thunk(f.as_raw(Private), <$t as CppTypeConversions>::into_insertelem(v.into_proxied(Private))) }
}
#[inline]
fn repeated_clear(mut f: Mut<Repeated<$t>>) {
Expand All @@ -493,8 +512,8 @@ macro_rules! impl_repeated_primitives {
unsafe { $get_thunk(f.as_raw(Private), i) })
}
#[inline]
unsafe fn repeated_set_unchecked(mut f: Mut<Repeated<$t>>, i: usize, v: View<$t>) {
unsafe { $set_thunk(f.as_raw(Private), i, v.into()) }
unsafe fn repeated_set_unchecked(mut f: Mut<Repeated<$t>>, i: usize, v: impl IntoProxied<$t>) {
unsafe { $set_thunk(f.as_raw(Private), i, <$t as CppTypeConversions>::into_insertelem(v.into_proxied(Private))) }
}
#[inline]
fn repeated_copy_from(src: View<Repeated<$t>>, mut dest: Mut<Repeated<$t>>) {
Expand Down Expand Up @@ -686,18 +705,18 @@ extern "C" {
}

macro_rules! impl_ProxiedInMapValue_for_non_generated_value_types {
($key_t:ty, $ffi_key_t:ty, $to_ffi_key:expr, $from_ffi_key:expr, for $($t:ty, $ffi_t:ty, $to_ffi_value:expr, $from_ffi_value:expr;)*) => {
($key_t:ty, $ffi_key_t:ty, $to_ffi_key:expr, $from_ffi_key:expr, for $($t:ty, $ffi_view_t:ty, $ffi_value_t:ty, $to_ffi_value:expr, $from_ffi_value:expr;)*) => {
paste! { $(
extern "C" {
fn [< proto2_rust_thunk_Map_ $key_t _ $t _new >]() -> RawMap;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _free >](m: RawMap);
fn [< proto2_rust_thunk_Map_ $key_t _ $t _clear >](m: RawMap);
fn [< proto2_rust_thunk_Map_ $key_t _ $t _size >](m: RawMap) -> usize;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _insert >](m: RawMap, key: $ffi_key_t, value: $ffi_t) -> bool;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _get >](m: RawMap, key: $ffi_key_t, value: *mut $ffi_t) -> bool;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _insert >](m: RawMap, key: $ffi_key_t, value: $ffi_value_t) -> bool;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _get >](m: RawMap, key: $ffi_key_t, value: *mut $ffi_view_t) -> bool;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _iter >](m: RawMap) -> UntypedMapIterator;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _iter_get >](iter: &mut UntypedMapIterator, key: *mut $ffi_key_t, value: *mut $ffi_t);
fn [< proto2_rust_thunk_Map_ $key_t _ $t _remove >](m: RawMap, key: $ffi_key_t, value: *mut $ffi_t) -> bool;
fn [< proto2_rust_thunk_Map_ $key_t _ $t _iter_get >](iter: &mut UntypedMapIterator, key: *mut $ffi_key_t, value: *mut $ffi_view_t);
fn [< proto2_rust_thunk_Map_ $key_t _ $t _remove >](m: RawMap, key: $ffi_key_t, value: *mut $ffi_view_t) -> bool;
}

impl ProxiedInMapValue<$key_t> for $t {
Expand Down Expand Up @@ -728,9 +747,9 @@ macro_rules! impl_ProxiedInMapValue_for_non_generated_value_types {
unsafe { [< proto2_rust_thunk_Map_ $key_t _ $t _size >](map.as_raw(Private)) }
}

fn map_insert(mut map: Mut<'_, Map<$key_t, Self>>, key: View<'_, $key_t>, value: View<'_, Self>) -> bool {
fn map_insert(mut map: Mut<'_, Map<$key_t, Self>>, key: View<'_, $key_t>, value: impl IntoProxied<Self>) -> bool {
let ffi_key = $to_ffi_key(key);
let ffi_value = $to_ffi_value(value);
let ffi_value = $to_ffi_value(value.into_proxied(Private));
unsafe { [< proto2_rust_thunk_Map_ $key_t _ $t _insert >](map.as_raw(Private), ffi_key, ffi_value) }
}

Expand Down Expand Up @@ -797,8 +816,14 @@ fn ptrlen_to_str<'msg>(val: PtrAndLen) -> &'msg ProtoStr {
unsafe { ProtoStr::from_utf8_unchecked(val.as_ref()) }
}

fn bytes_to_ptrlen(val: &[u8]) -> PtrAndLen {
val.into()
fn protostr_into_cppstdstring(val: ProtoString) -> CppStdString {
let m = ManuallyDrop::new(val);
m.inner.owned_ptr
}

fn protobytes_into_cppstdstring(val: ProtoBytes) -> CppStdString {
let m = ManuallyDrop::new(val);
m.inner.owned_ptr
}

// Warning: this function is unsound on its own! `val.as_ref()` must be safe to
Expand All @@ -813,15 +838,15 @@ macro_rules! impl_ProxiedInMapValue_for_key_types {
$(
impl_ProxiedInMapValue_for_non_generated_value_types!(
$t, $ffi_t, $to_ffi_key, $from_ffi_key, for
f32, f32, identity, identity;
f64, f64, identity, identity;
i32, i32, identity, identity;
u32, u32, identity, identity;
i64, i64, identity, identity;
u64, u64, identity, identity;
bool, bool, identity, identity;
ProtoString, PtrAndLen, str_to_ptrlen, ptrlen_to_str;
ProtoBytes, PtrAndLen, bytes_to_ptrlen, ptrlen_to_bytes;
f32, f32, f32, identity, identity;
f64, f64, f64, identity, identity;
i32, i32, i32, identity, identity;
u32, u32, u32, identity, identity;
i64, i64, i64, identity, identity;
u64, u64, u64, identity, identity;
bool, bool, bool, identity, identity;
ProtoString, PtrAndLen, CppStdString, protostr_into_cppstdstring, ptrlen_to_str;
ProtoBytes, PtrAndLen, CppStdString, protobytes_into_cppstdstring, ptrlen_to_bytes;
);
)*
}
Expand Down
31 changes: 16 additions & 15 deletions rust/cpp_kernel/map.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <cstdint>
#include <string>
#include <utility>

#include "google/protobuf/map.h"
#include "rust/cpp_kernel/strings.h"
Expand All @@ -13,27 +14,27 @@ void proto2_rust_thunk_UntypedMapIterator_increment(
iter->PlusPlus();
}

__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(int32_t, i32, int32_t, value,
cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(int32_t, i32, int32_t,
int32_t, value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(uint32_t, u32, uint32_t,
uint32_t, value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(float, f32, float, float,
value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(float, f32, float, value,
cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(double, f64, double, value,
cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(bool, bool, bool, value,
cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(uint64_t, u64, uint64_t,
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(double, f64, double, double,
value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(bool, bool, bool, bool,
value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(int64_t, i64, int64_t, value,
cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(uint64_t, u64, uint64_t,
uint64_t, value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(int64_t, i64, int64_t,
int64_t, value, cpp_value);
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(
std::string, ProtoBytes, google::protobuf::rust::PtrAndLen,
std::string(value.ptr, value.len),
std::string, ProtoBytes, google::protobuf::rust::PtrAndLen, std::string*,
std::move(*value),
google::protobuf::rust::PtrAndLen(cpp_value.data(), cpp_value.size()));
__PB_RUST_EXPOSE_SCALAR_MAP_METHODS_FOR_VALUE_TYPE(
std::string, ProtoString, google::protobuf::rust::PtrAndLen,
std::string(value.ptr, value.len),
std::string, ProtoString, google::protobuf::rust::PtrAndLen, std::string*,
std::move(*value),
google::protobuf::rust::PtrAndLen(cpp_value.data(), cpp_value.size()));

} // extern "C"
Loading

0 comments on commit bd2fe9c

Please sign in to comment.