From 74ff001424d942f408a3f29dc9a10714004fe3ae Mon Sep 17 00:00:00 2001 From: Alyssa Haroldsen Date: Fri, 8 Dec 2023 18:08:49 -0800 Subject: [PATCH] Remove iteration and item mutators from Repeated This significantly simplifies the internals of PrimitiveMut, and removes the need to refactor BytesMut and ProtoStrMut to have the same runtime branching. PiperOrigin-RevId: 589292565 --- rust/primitive.rs | 57 +++---------------- rust/repeated.rs | 41 ------------- rust/shared.rs | 2 +- rust/test/shared/accessors_repeated_test.rs | 27 ++++----- .../rust/accessors/singular_scalar.cc | 2 +- src/google/protobuf/compiler/rust/message.cc | 2 +- 6 files changed, 21 insertions(+), 110 deletions(-) diff --git a/rust/primitive.rs b/rust/primitive.rs index 839dfc0ce8..6d0e235381 100644 --- a/rust/primitive.rs +++ b/rust/primitive.rs @@ -7,7 +7,6 @@ use crate::__internal::Private; use crate::__runtime::InnerPrimitiveMut; -use crate::repeated::RepeatedMut; use crate::vtable::{ PrimitiveOptionalMutVTable, PrimitiveVTable, ProxiedWithRawOptionalVTable, ProxiedWithRawVTable, RawVTableOptionalMutatorData, @@ -15,31 +14,18 @@ use crate::vtable::{ use crate::{Mut, MutProxy, Proxied, ProxiedWithPresence, SettableValue, View, ViewProxy}; #[derive(Debug)] -pub struct SingularPrimitiveMut<'a, T: ProxiedWithRawVTable> { +pub struct PrimitiveMut<'a, T: ProxiedWithRawVTable> { inner: InnerPrimitiveMut<'a, T>, } -#[derive(Debug)] -pub enum PrimitiveMut<'a, T: ProxiedWithRawVTable> { - Singular(SingularPrimitiveMut<'a, T>), - Repeated(RepeatedMut<'a, T>, usize), -} - impl<'a, T: ProxiedWithRawVTable> PrimitiveMut<'a, T> { - #[doc(hidden)] - pub fn from_singular(_private: Private, inner: InnerPrimitiveMut<'a, T>) -> Self { - PrimitiveMut::Singular(SingularPrimitiveMut::from_inner(_private, inner)) - } -} - -impl<'a, T: ProxiedWithRawVTable> SingularPrimitiveMut<'a, T> { #[doc(hidden)] pub fn from_inner(_private: Private, inner: InnerPrimitiveMut<'a, T>) -> Self { Self { inner } } } -unsafe impl<'a, T: ProxiedWithRawVTable> Sync for SingularPrimitiveMut<'a, T> {} +unsafe impl<'a, T: ProxiedWithRawVTable> Sync for PrimitiveMut<'a, T> {} macro_rules! impl_singular_primitives { ($($t:ty),*) => { @@ -63,14 +49,7 @@ macro_rules! impl_singular_primitives { impl<'a> PrimitiveMut<'a, $t> { pub fn get(&self) -> View<'_, $t> { - match self { - PrimitiveMut::Singular(s) => { - s.get() - } - PrimitiveMut::Repeated(r, i) => { - r.get().get(*i).unwrap() - } - } + self.inner.get() } pub fn set(&mut self, val: impl SettableValue<$t>) { @@ -98,14 +77,7 @@ macro_rules! impl_singular_primitives { impl<'a> MutProxy<'a> for PrimitiveMut<'a, $t> { fn as_mut(&mut self) -> Mut<'_, Self::Proxied> { - match self { - PrimitiveMut::Singular(s) => { - PrimitiveMut::Singular(s.as_mut()) - } - PrimitiveMut::Repeated(r, i) => { - PrimitiveMut::Repeated(r.as_mut(), *i) - } - } + PrimitiveMut { inner: self.inner } } fn into_mut<'shorter>(self) -> Mut<'shorter, Self::Proxied> @@ -117,23 +89,8 @@ macro_rules! impl_singular_primitives { impl SettableValue<$t> for $t { fn set_on<'a>(self, _private: Private, mutator: Mut<'a, $t>) where $t: 'a { - match mutator { - PrimitiveMut::Singular(s) => { - unsafe { (s.inner).set(self) }; - } - PrimitiveMut::Repeated(mut r, i) => { - r.set(i, self); - } - } - } - } - - impl<'a> SingularPrimitiveMut<'a, $t> { - pub fn get(&self) -> $t { - self.inner.get() - } - pub fn as_mut(&mut self) -> SingularPrimitiveMut<'_, $t> { - SingularPrimitiveMut::from_inner(Private, self.inner) + // SAFETY: the raw mutator is valid for `'a` as enforced by `Mut` + unsafe { mutator.inner.set(self) } } } @@ -148,7 +105,7 @@ macro_rules! impl_singular_primitives { } fn make_mut(_private: Private, inner: InnerPrimitiveMut<'_, Self>) -> Mut<'_, Self> { - PrimitiveMut::Singular(SingularPrimitiveMut::from_inner(Private, inner)) + PrimitiveMut::from_inner(Private, inner) } } diff --git a/rust/repeated.rs b/rust/repeated.rs index defab4452f..19b64bc48e 100644 --- a/rust/repeated.rs +++ b/rust/repeated.rs @@ -15,7 +15,6 @@ use crate::{ Mut, MutProxy, Proxied, SettableValue, View, ViewProxy, __internal::{Private, RawRepeatedField}, __runtime::{RepeatedField, RepeatedFieldInner}, - primitive::PrimitiveMut, vtable::ProxiedWithRawVTable, }; @@ -82,11 +81,6 @@ impl<'a, T> std::ops::Deref for RepeatedMut<'a, T> { } } -pub struct RepeatedFieldIterMut<'a, T> { - inner: RepeatedMut<'a, T>, - current_index: usize, -} - pub struct Repeated(PhantomData); macro_rules! impl_repeated_primitives { @@ -167,18 +161,9 @@ macro_rules! impl_repeated_primitives { pub fn set(&mut self, index: usize, val: $t) { self.inner.set(index, val) } - pub fn get_mut(&mut self, index: usize) -> Option> { - if index >= self.len() { - return None; - } - Some(PrimitiveMut::Repeated(self.as_mut(), index)) - } pub fn iter(&self) -> RepeatedFieldIter<'_, $t> { self.as_view().into_iter() } - pub fn iter_mut(&mut self) -> RepeatedFieldIterMut<'_, $t> { - self.as_mut().into_iter() - } pub fn copy_from(&mut self, src: RepeatedView<'_, $t>) { self.inner.copy_from(&src.inner); } @@ -202,32 +187,6 @@ macro_rules! impl_repeated_primitives { RepeatedFieldIter { inner: self.inner, current_index: 0 } } } - - impl <'a> std::iter::Iterator for RepeatedFieldIterMut<'a, $t> { - type Item = Mut<'a, $t>; - fn next(&mut self) -> Option { - if self.current_index >= self.inner.len() { - return None; - } - let elem = PrimitiveMut::Repeated( - // While this appears to allow mutable aliasing - // (multiple `Self::Item`s can co-exist), each `Item` - // only references a specific unique index. - RepeatedMut{ inner: self.inner.inner }, - self.current_index, - ); - self.current_index += 1; - Some(elem) - } - } - - impl<'a> std::iter::IntoIterator for RepeatedMut<'a, $t> { - type Item = Mut<'a, $t>; - type IntoIter = RepeatedFieldIterMut<'a, $t>; - fn into_iter(self) -> Self::IntoIter { - RepeatedFieldIterMut { inner: self, current_index: 0 } - } - } )* } } diff --git a/rust/shared.rs b/rust/shared.rs index 1ab53cb97c..f94f987989 100644 --- a/rust/shared.rs +++ b/rust/shared.rs @@ -19,7 +19,7 @@ use std::fmt; pub mod __public { pub use crate::map::{Map, MapMut, MapView}; pub use crate::optional::{AbsentField, FieldEntry, Optional, PresentField}; - pub use crate::primitive::{PrimitiveMut, SingularPrimitiveMut}; + pub use crate::primitive::PrimitiveMut; pub use crate::proxied::{ Mut, MutProxy, Proxied, ProxiedWithPresence, SettableValue, View, ViewProxy, }; diff --git a/rust/test/shared/accessors_repeated_test.rs b/rust/test/shared/accessors_repeated_test.rs index f88dbf090c..d0b9a3bc42 100644 --- a/rust/test/shared/accessors_repeated_test.rs +++ b/rust/test/shared/accessors_repeated_test.rs @@ -27,12 +27,9 @@ macro_rules! generate_repeated_numeric_test { mutator.push(1 as $t); mutator.push(3 as $t); - assert_that!(mutator.get_mut(2).is_some(), eq(true)); - let mut mut_elem = mutator.get_mut(2).unwrap(); - mut_elem.set(4 as $t); - assert_that!(mut_elem.get(), eq(4 as $t)); - mut_elem.clear(); - assert_that!(mut_elem.get(), eq(0 as $t)); + mutator.set(2, 4 as $t); + assert_that!(mutator.get(2), some(eq(4 as $t))); + mutator.set(2, 0 as $t); assert_that!( mutator.iter().collect::>(), @@ -43,8 +40,8 @@ macro_rules! generate_repeated_numeric_test { eq(vec![2 as $t, 1 as $t, 0 as $t]) ); - for mut mutable_elem in msg.[]() { - mutable_elem.set(0 as $t); + for i in 0..mutator.len() { + mutator.set(i, 0 as $t); } assert_that!( msg.[]().iter().all(|v| v == (0 as $t)), @@ -96,18 +93,16 @@ fn test_repeated_bool_accessors() { mutator.push(true); mutator.push(false); - assert_that!(mutator.get_mut(2), pat!(Some(_))); - let mut mut_elem = mutator.get_mut(2).unwrap(); - mut_elem.set(true); - assert_that!(mut_elem.get(), eq(true)); - mut_elem.clear(); - assert_that!(mut_elem.get(), eq(false)); + mutator.set(2, true); + assert_that!(mutator.get(2), some(eq(true))); + mutator.set(2, false); + assert_that!(mutator.get(2), some(eq(false))); assert_that!(mutator.iter().collect::>(), eq(vec![false, true, false])); assert_that!((*mutator).into_iter().collect::>(), eq(vec![false, true, false])); - for mut mutable_elem in msg.repeated_bool_mut() { - mutable_elem.set(false); + for i in 0..mutator.len() { + mutator.set(i, false); } assert_that!(msg.repeated_bool().iter().all(|v| v), eq(false)); } diff --git a/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc b/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc index 1dc00b1d9c..23cdb2a246 100644 --- a/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc +++ b/src/google/protobuf/compiler/rust/accessors/singular_scalar.cc @@ -98,7 +98,7 @@ void SingularScalar::InMsgImpl(Context field) const { $setter_thunk$, ); - $pb$::PrimitiveMut::from_singular( + $pb$::PrimitiveMut::from_inner( $pbi$::Private, unsafe { $pbi$::RawVTableMutator::new( diff --git a/src/google/protobuf/compiler/rust/message.cc b/src/google/protobuf/compiler/rust/message.cc index a1cfb4e8c4..c98b6f409e 100644 --- a/src/google/protobuf/compiler/rust/message.cc +++ b/src/google/protobuf/compiler/rust/message.cc @@ -263,7 +263,7 @@ void GetterForViewOrMut(Context field, bool is_mut) { $pbi$::Private, $getter_thunk$, $setter_thunk$); - $pb$::PrimitiveMut::from_singular( + $pb$::PrimitiveMut::from_inner( $pbi$::Private, unsafe { $pbi$::RawVTableMutator::new($pbi$::Private,