Avoid bevy_reflect::List::iter wrapping in release mode (#13271)
# Objective Fixes #13230 ## Solution Uses solution described in #13230 They mention a worry about adding a branch, but I'm not sure there is one. This code ```Rust #[no_mangle] pub fn next_if_some(num: i32, b: Option<bool>) -> i32 { num + b.is_some() as i32 } ``` produces this assembly with opt-level 3 ```asm next_if_some: xor eax, eax cmp sil, 2 setne al add eax, edi ret ``` ## Testing Added test from #13230, tagged it as ignore as it is only useful in release mode and very slow if you accidentally invoke it in debug mode. --- ## Changelog Iterationg of ListIter will no longer overflow and wrap around ## Migration Guide
This commit is contained in:
		
							parent
							
								
									443ce9a62b
								
							
						
					
					
						commit
						278380394f
					
				| @ -378,7 +378,7 @@ impl<'a> Iterator for ArrayIter<'a> { | |||||||
|     #[inline] |     #[inline] | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.array.get(self.index); |         let value = self.array.get(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -503,3 +503,32 @@ pub fn array_debug(dyn_array: &dyn Array, f: &mut std::fmt::Formatter<'_>) -> st | |||||||
|     } |     } | ||||||
|     debug.finish() |     debug.finish() | ||||||
| } | } | ||||||
|  | #[cfg(test)] | ||||||
|  | mod tests { | ||||||
|  |     use crate::{Reflect, ReflectRef}; | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         const SIZE: usize = if cfg!(debug_assertions) { | ||||||
|  |             4 | ||||||
|  |         } else { | ||||||
|  |             // If compiled in release mode, verify we dont overflow
 | ||||||
|  |             usize::MAX | ||||||
|  |         }; | ||||||
|  | 
 | ||||||
|  |         let b = Box::new([(); SIZE]).into_reflect(); | ||||||
|  | 
 | ||||||
|  |         let ReflectRef::Array(array) = b.reflect_ref() else { | ||||||
|  |             panic!("Not an array..."); | ||||||
|  |         }; | ||||||
|  | 
 | ||||||
|  |         let mut iter = array.iter(); | ||||||
|  |         iter.index = SIZE - 1; | ||||||
|  |         assert!(iter.next().is_some()); | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert!(iter.index == SIZE); | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert!(iter.index == SIZE); | ||||||
|  |     } | ||||||
|  | } | ||||||
|  | |||||||
| @ -280,7 +280,7 @@ impl<'a> Iterator for VariantFieldIter<'a> { | |||||||
|                 Some(VariantField::Struct(name, self.container.field(name)?)) |                 Some(VariantField::Struct(name, self.container.field(name)?)) | ||||||
|             } |             } | ||||||
|         }; |         }; | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -312,3 +312,58 @@ impl<'a> VariantField<'a> { | |||||||
|         } |         } | ||||||
|     } |     } | ||||||
| } | } | ||||||
|  | 
 | ||||||
|  | // Tests that need access to internal fields have to go here rather than in mod.rs
 | ||||||
|  | #[cfg(test)] | ||||||
|  | mod tests { | ||||||
|  |     use crate as bevy_reflect; | ||||||
|  |     use crate::*; | ||||||
|  | 
 | ||||||
|  |     #[derive(Reflect, Debug, PartialEq)] | ||||||
|  |     enum MyEnum { | ||||||
|  |         A, | ||||||
|  |         B(usize, i32), | ||||||
|  |         C { foo: f32, bar: bool }, | ||||||
|  |     } | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         // unit enums always return none, so index should stay at 0
 | ||||||
|  |         let unit_enum = MyEnum::A; | ||||||
|  |         let mut iter = unit_enum.iter_fields(); | ||||||
|  |         let size = iter.len(); | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             assert!(iter.next().is_none()); | ||||||
|  |             assert_eq!(size, iter.index); | ||||||
|  |         } | ||||||
|  |         // tuple enums we iter over each value (unnamed fields), stop after that
 | ||||||
|  |         let tuple_enum = MyEnum::B(0, 1); | ||||||
|  |         let mut iter = tuple_enum.iter_fields(); | ||||||
|  |         let size = iter.len(); | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             let prev_index = iter.index; | ||||||
|  |             assert!(iter.next().is_some()); | ||||||
|  |             assert_eq!(prev_index, iter.index - 1); | ||||||
|  |         } | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             assert!(iter.next().is_none()); | ||||||
|  |             assert_eq!(size, iter.index); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|  |         // struct enums, we iterate over each field in the struct
 | ||||||
|  |         let struct_enum = MyEnum::C { | ||||||
|  |             foo: 0., | ||||||
|  |             bar: false, | ||||||
|  |         }; | ||||||
|  |         let mut iter = struct_enum.iter_fields(); | ||||||
|  |         let size = iter.len(); | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             let prev_index = iter.index; | ||||||
|  |             assert!(iter.next().is_some()); | ||||||
|  |             assert_eq!(prev_index, iter.index - 1); | ||||||
|  |         } | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             assert!(iter.next().is_none()); | ||||||
|  |             assert_eq!(size, iter.index); | ||||||
|  |         } | ||||||
|  |     } | ||||||
|  | } | ||||||
|  | |||||||
| @ -405,7 +405,7 @@ impl<'a> Iterator for ListIter<'a> { | |||||||
|     #[inline] |     #[inline] | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.list.get(self.index); |         let value = self.list.get(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -533,6 +533,7 @@ pub fn list_debug(dyn_list: &dyn List, f: &mut Formatter<'_>) -> std::fmt::Resul | |||||||
| #[cfg(test)] | #[cfg(test)] | ||||||
| mod tests { | mod tests { | ||||||
|     use super::DynamicList; |     use super::DynamicList; | ||||||
|  |     use crate::{Reflect, ReflectRef}; | ||||||
|     use std::assert_eq; |     use std::assert_eq; | ||||||
| 
 | 
 | ||||||
|     #[test] |     #[test] | ||||||
| @ -547,4 +548,29 @@ mod tests { | |||||||
|             assert_eq!(index, value); |             assert_eq!(index, value); | ||||||
|         } |         } | ||||||
|     } |     } | ||||||
|  | 
 | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         const SIZE: usize = if cfg!(debug_assertions) { | ||||||
|  |             4 | ||||||
|  |         } else { | ||||||
|  |             // If compiled in release mode, verify we dont overflow
 | ||||||
|  |             usize::MAX | ||||||
|  |         }; | ||||||
|  |         let b = Box::new(vec![(); SIZE]).into_reflect(); | ||||||
|  | 
 | ||||||
|  |         let ReflectRef::List(list) = b.reflect_ref() else { | ||||||
|  |             panic!("Not a list..."); | ||||||
|  |         }; | ||||||
|  | 
 | ||||||
|  |         let mut iter = list.iter(); | ||||||
|  |         iter.index = SIZE - 1; | ||||||
|  |         assert!(iter.next().is_some()); | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert!(iter.index == SIZE); | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert!(iter.index == SIZE); | ||||||
|  |     } | ||||||
| } | } | ||||||
|  | |||||||
| @ -417,7 +417,7 @@ impl<'a> Iterator for MapIter<'a> { | |||||||
| 
 | 
 | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.map.get_at(self.index); |         let value = self.map.get_at(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -618,4 +618,27 @@ mod tests { | |||||||
| 
 | 
 | ||||||
|         assert!(map.get_at(2).is_none()); |         assert!(map.get_at(2).is_none()); | ||||||
|     } |     } | ||||||
|  | 
 | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         let values = ["first", "last"]; | ||||||
|  |         let mut map = DynamicMap::default(); | ||||||
|  |         map.insert(0usize, values[0]); | ||||||
|  |         map.insert(1usize, values[1]); | ||||||
|  | 
 | ||||||
|  |         let mut iter = map.iter(); | ||||||
|  |         let size = iter.len(); | ||||||
|  | 
 | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             let prev_index = iter.index; | ||||||
|  |             assert!(iter.next().is_some()); | ||||||
|  |             assert_eq!(prev_index, iter.index - 1); | ||||||
|  |         } | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         for _ in 0..2 { | ||||||
|  |             assert!(iter.next().is_none()); | ||||||
|  |             assert_eq!(size, iter.index); | ||||||
|  |         } | ||||||
|  |     } | ||||||
| } | } | ||||||
|  | |||||||
| @ -204,7 +204,7 @@ impl<'a> Iterator for FieldIter<'a> { | |||||||
| 
 | 
 | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.struct_val.field_at(self.index); |         let value = self.struct_val.field_at(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -562,3 +562,31 @@ pub fn struct_debug(dyn_struct: &dyn Struct, f: &mut Formatter<'_>) -> std::fmt: | |||||||
|     } |     } | ||||||
|     debug.finish() |     debug.finish() | ||||||
| } | } | ||||||
|  | 
 | ||||||
|  | #[cfg(test)] | ||||||
|  | mod tests { | ||||||
|  |     use crate as bevy_reflect; | ||||||
|  |     use crate::*; | ||||||
|  |     #[derive(Reflect, Default)] | ||||||
|  |     struct MyStruct { | ||||||
|  |         a: (), | ||||||
|  |         b: (), | ||||||
|  |         c: (), | ||||||
|  |     } | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         let my_struct = MyStruct::default(); | ||||||
|  |         let mut iter = my_struct.iter_fields(); | ||||||
|  |         iter.index = iter.len() - 1; | ||||||
|  |         let prev_index = iter.index; | ||||||
|  |         assert!(iter.next().is_some()); | ||||||
|  |         assert_eq!(prev_index, iter.index - 1); | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         let prev_index = iter.index; | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(prev_index, iter.index); | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(prev_index, iter.index); | ||||||
|  |     } | ||||||
|  | } | ||||||
|  | |||||||
| @ -75,7 +75,7 @@ impl<'a> Iterator for TupleFieldIter<'a> { | |||||||
| 
 | 
 | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.tuple.field(self.index); |         let value = self.tuple.field(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -709,3 +709,24 @@ macro_rules! impl_type_path_tuple { | |||||||
| } | } | ||||||
| 
 | 
 | ||||||
| all_tuples!(impl_type_path_tuple, 0, 12, P); | all_tuples!(impl_type_path_tuple, 0, 12, P); | ||||||
|  | 
 | ||||||
|  | #[cfg(test)] | ||||||
|  | mod tests { | ||||||
|  |     use super::Tuple; | ||||||
|  | 
 | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         let mut iter = (0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11).iter_fields(); | ||||||
|  |         let size = iter.len(); | ||||||
|  |         iter.index = size - 1; | ||||||
|  |         let prev_index = iter.index; | ||||||
|  |         assert!(iter.next().is_some()); | ||||||
|  |         assert_eq!(prev_index, iter.index - 1); | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(size, iter.index); | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(size, iter.index); | ||||||
|  |     } | ||||||
|  | } | ||||||
|  | |||||||
| @ -155,7 +155,7 @@ impl<'a> Iterator for TupleStructFieldIter<'a> { | |||||||
| 
 | 
 | ||||||
|     fn next(&mut self) -> Option<Self::Item> { |     fn next(&mut self) -> Option<Self::Item> { | ||||||
|         let value = self.tuple_struct.field(self.index); |         let value = self.tuple_struct.field(self.index); | ||||||
|         self.index += 1; |         self.index += value.is_some() as usize; | ||||||
|         value |         value | ||||||
|     } |     } | ||||||
| 
 | 
 | ||||||
| @ -475,3 +475,26 @@ pub fn tuple_struct_debug( | |||||||
|     } |     } | ||||||
|     debug.finish() |     debug.finish() | ||||||
| } | } | ||||||
|  | 
 | ||||||
|  | #[cfg(test)] | ||||||
|  | mod tests { | ||||||
|  |     use crate as bevy_reflect; | ||||||
|  |     use crate::*; | ||||||
|  |     #[derive(Reflect)] | ||||||
|  |     struct Ts(u8, u8, u8, u8, u8, u8, u8, u8, u8, u8, u8, u8); | ||||||
|  |     #[test] | ||||||
|  |     fn next_index_increment() { | ||||||
|  |         let mut iter = Ts(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11).iter_fields(); | ||||||
|  |         let size = iter.len(); | ||||||
|  |         iter.index = size - 1; | ||||||
|  |         let prev_index = iter.index; | ||||||
|  |         assert!(iter.next().is_some()); | ||||||
|  |         assert_eq!(prev_index, iter.index - 1); | ||||||
|  | 
 | ||||||
|  |         // When None we should no longer increase index
 | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(size, iter.index); | ||||||
|  |         assert!(iter.next().is_none()); | ||||||
|  |         assert_eq!(size, iter.index); | ||||||
|  |     } | ||||||
|  | } | ||||||
|  | |||||||
		Loading…
	
		Reference in New Issue
	
	Block a user
	 rmsthebest
						rmsthebest