From 333fab38b47f611422a74015fa6256be76ea7208 Mon Sep 17 00:00:00 2001 From: Filipe Rodrigues Date: Fri, 18 Sep 2020 17:24:08 +0100 Subject: [PATCH] Extended interface of `AsciiStrArr`. `AsciiStrArr` now stores it's chracters as `MaybeUninit`. Added unit tests to `AsciiStrArr`. Fixed documentaton with `address::real::ToDataError`. --- src/ascii_str_arr.rs | 155 +++++++++++++++++++++++++++++++++---- src/ascii_str_arr/slice.rs | 10 ++- src/ascii_str_arr/test.rs | 52 +++++++++++++ src/io/address/real.rs | 2 +- src/lib.rs | 9 ++- 5 files changed, 205 insertions(+), 23 deletions(-) create mode 100644 src/ascii_str_arr/test.rs diff --git a/src/ascii_str_arr.rs b/src/ascii_str_arr.rs index 8a6c5a0..1c256af 100644 --- a/src/ascii_str_arr.rs +++ b/src/ascii_str_arr.rs @@ -3,6 +3,8 @@ // Modules pub mod error; pub mod slice; +#[cfg(test)] +mod test; mod visitor; // Exports @@ -11,22 +13,39 @@ pub use slice::SliceIndex; // Imports use ascii::{AsciiChar, AsciiStr}; -use std::{cmp::Ordering, convert::TryFrom, fmt, hash::Hash}; +use std::{cmp::Ordering, convert::TryFrom, fmt, hash::Hash, mem::MaybeUninit}; use visitor::DeserializerVisitor; /// An ascii string backed by an array -#[derive(Eq, Clone, Copy)] +#[derive(Clone, Copy)] pub struct AsciiStrArr { /// Characters - chars: [AsciiChar; N], + // Invariant + chars: [MaybeUninit; N], /// Size // Invariant `self.len <= N` + // Note: On all methods except `Self::len()`, one should call `self.len()` instead of using `self.len`. len: usize, } -// Length interface +// Constructors impl AsciiStrArr { + /// Creates a new empty string + #[must_use] + pub fn new() -> Self { + Self { + chars: MaybeUninit::uninit_array(), + len: 0, + } + } +} + +/// String lengths +impl AsciiStrArr { + /// The capacity of the string + pub const CAPACITY: usize = N; + /// Returns the length of this string #[must_use] #[contracts::debug_ensures(self.len <= N)] @@ -34,6 +53,22 @@ impl AsciiStrArr { self.len } + /// Returns the capacity of the string, `N` + #[must_use] + pub const fn capacity() -> usize { + Self::CAPACITY + } + + /// Sets this string's length + /// + /// # Safety + /// - All elements `0..new_len` must be initialized. + /// - `new_len` must be less or equal to `N`. + #[contracts::debug_requires(new_len <= N)] + pub unsafe fn set_len(&mut self, new_len: usize) { + self.len = new_len; + } + /// Returns if this string is empty #[must_use] pub fn is_empty(&self) -> bool { @@ -41,23 +76,47 @@ impl AsciiStrArr { } } -// Conversions into other strings +/// Conversions to other string types impl AsciiStrArr { /// Converts this string to a `&AsciiStr` #[must_use] pub fn as_ascii(&self) -> &AsciiStr { + // Get all the initialized elements // SAFETY: `self.len <= N` let chars = unsafe { self.chars.get_unchecked(..self.len()) }; - chars.into() + + // Then get a reference to them + // SAFETY: The first `self.len` elements are initialized + let chars = unsafe { MaybeUninit::slice_assume_init_ref(chars) }; + + <&AsciiStr>::from(chars) } /// Converts this string to a `&mut AsciiStr` #[must_use] pub fn as_ascii_mut(&mut self) -> &mut AsciiStr { + // Get all the initialized elements // SAFETY: `self.len <= N` let len = self.len(); let chars = unsafe { self.chars.get_unchecked_mut(..len) }; - chars.into() + + // Then get a mutable reference to them + // SAFETY: The first `self.len` elements are initialized + let chars = unsafe { MaybeUninit::slice_assume_init_mut(chars) }; + + <&mut AsciiStr>::from(chars) + } + + /// Converts this string to a `&[AsciiChar]` + #[must_use] + pub fn as_ascii_slice(&self) -> &[AsciiChar] { + self.as_ascii().as_slice() + } + + /// Converts this string to a `&mut [AsciiChar]` + #[must_use] + pub fn as_ascii_slice_mut(&mut self) -> &mut [AsciiChar] { + self.as_ascii_mut().as_mut_slice() } /// Converts this string to a `&[u8]` @@ -71,26 +130,62 @@ impl AsciiStrArr { pub fn as_str(&self) -> &str { self.as_ascii().as_str() } + + /// Returns a pointer to the initialized elements + /// + /// # Safety + /// The returned pointer is only valid for `self.len()` elements + /// The returned pointer is invalidated if `self` is moved. + #[must_use] + pub fn as_ptr(&self) -> *const AsciiChar { + self.as_ascii().as_ptr() + } + + /// Returns a mutable pointer to the initialized elements + /// + /// # Safety + /// The returned pointer is only valid for `self.len()` elements + /// The returned pointer is invalidated if `self` is moved. + #[must_use] + pub fn as_ptr_mut(&mut self) -> *mut AsciiChar { + self.as_ascii_mut().as_mut_ptr() + } + + /// Exposes the underlying buffer this string contains + /// + /// # Safety + /// All elements `0..self.len()` must remain initialized. + pub unsafe fn buffer_mut(&mut self) -> &mut [MaybeUninit; N] { + &mut self.chars + } } /// Conversions from other strings impl AsciiStrArr { - /// Creates a string from a `&AsciiStr` - pub fn from_ascii(ascii: &AsciiStr) -> Result> { - // If we can't fit it, return Err + /// Creates a string from anything that coerces to `&[AsciiChar]`, including `AsciiStr` + pub fn from_ascii>(ascii: &S) -> Result> { + let ascii = ascii.as_ref(); + + // If it has too many elements, return Err let len = ascii.len(); if len > N { return Err(TooLongError::); } - // Else copy everything over and return ourselves - let mut chars = [AsciiChar::Null; N]; - chars[0..len].copy_from_slice(ascii.as_ref()); + // Else create an uninitialized array and copy over the initialized characters + let mut chars = MaybeUninit::uninit_array(); + for (idx, c) in chars.iter_mut().take(len).enumerate() { + // Write each character + // SAFETY: `idx` is within bounds (`0..len`) + // Note: `MaybeUnit::drop` is a no-op, so we can use normal assignment. + *c = MaybeUninit::new(*unsafe { ascii.get_unchecked(idx) }); + } + Ok(Self { chars, len }) } /// Creates a string from a `&[u8]` - pub fn from_bytes(bytes: &[u8]) -> Result> { + pub fn from_bytes>(bytes: &B) -> Result> { // Get the bytes as ascii first let ascii = AsciiStr::from_ascii(bytes).map_err(FromBytesError::NotAscii)?; @@ -101,7 +196,7 @@ impl AsciiStrArr { // Note: No `from_str`, implemented using `FromStr` } -// Slicing +/// Slicing impl AsciiStrArr { /// Slices this string, if in bounds #[must_use] @@ -122,7 +217,8 @@ impl AsciiStrArr { /// reference is not used #[must_use] pub unsafe fn get_unchecked(&self, idx: I) -> &I::Output { - idx.get_unchecked(self) + // SAFETY: Index is guaranteed to be in bounds by the caller + unsafe { idx.get_unchecked(self) } } /// Slices the string mutably without checking bounds @@ -132,7 +228,8 @@ impl AsciiStrArr { /// reference is not used #[must_use] pub unsafe fn get_unchecked_mut(&mut self, idx: I) -> &mut I::Output { - idx.get_unchecked_mut(self) + // SAFETY: Index is guaranteed to be in bounds by the caller + unsafe { idx.get_unchecked_mut(self) } } } @@ -148,6 +245,18 @@ impl AsMut for AsciiStrArr { } } +impl AsRef<[AsciiChar]> for AsciiStrArr { + fn as_ref(&self) -> &[AsciiChar] { + self.as_ascii_slice() + } +} + +impl AsMut<[AsciiChar]> for AsciiStrArr { + fn as_mut(&mut self) -> &mut [AsciiChar] { + self.as_ascii_slice_mut() + } +} + impl AsRef<[u8]> for AsciiStrArr { fn as_ref(&self) -> &[u8] { self.as_bytes() @@ -160,12 +269,17 @@ impl AsRef for AsciiStrArr { } } +// Note: No `AsMut<[u8]>` nor `AsMut`, as that'd allow for modification +// outside of ascii. + impl PartialEq for AsciiStrArr { fn eq(&self, other: &Self) -> bool { AsciiStr::eq(self.as_ascii(), other.as_ascii()) } } +impl Eq for AsciiStrArr {} + impl PartialOrd for AsciiStrArr { fn partial_cmp(&self, other: &Self) -> Option { AsciiStr::partial_cmp(self.as_ascii(), other.as_ascii()) @@ -184,6 +298,12 @@ impl Hash for AsciiStrArr { } } +impl Default for AsciiStrArr { + fn default() -> Self { + Self::new() + } +} + impl fmt::Debug for AsciiStrArr { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { AsciiStr::fmt(self.as_ascii(), f) @@ -227,6 +347,7 @@ impl From<&[AsciiChar; N]> for AsciiStrArr { // TODO: Generalize this to `impl From<[AsciiChar; M]> for AsciiStrArr where M <= N` impl From<[AsciiChar; N]> for AsciiStrArr { fn from(chars: [AsciiChar; N]) -> Self { + let chars = chars.map(MaybeUninit::new); Self { chars, len: N } } } diff --git a/src/ascii_str_arr/slice.rs b/src/ascii_str_arr/slice.rs index 236a3e1..ab45502 100644 --- a/src/ascii_str_arr/slice.rs +++ b/src/ascii_str_arr/slice.rs @@ -39,19 +39,21 @@ where type Output = >::Output; fn get(self, ascii_str: &AsciiStrArr) -> Option<&Self::Output> { - ascii_str.as_ascii().as_slice().get(self) + ascii_str.as_ascii_slice().get(self) } fn get_mut(self, ascii_str: &mut AsciiStrArr) -> Option<&mut Self::Output> { - ascii_str.as_ascii_mut().as_mut_slice().get_mut(self) + ascii_str.as_ascii_slice_mut().get_mut(self) } unsafe fn get_unchecked(self, ascii_str: &AsciiStrArr) -> &Self::Output { - ascii_str.as_ascii().as_slice().get_unchecked(self) + // SAFETY: Index is guaranteed to be in bounds by the caller + unsafe { ascii_str.as_ascii_slice().get_unchecked(self) } } unsafe fn get_unchecked_mut(self, ascii_str: &mut AsciiStrArr) -> &mut Self::Output { - ascii_str.as_ascii_mut().as_mut_slice().get_unchecked_mut(self) + // SAFETY: Index is guaranteed to be in bounds by the caller + unsafe { ascii_str.as_ascii_slice_mut().get_unchecked_mut(self) } } } diff --git a/src/ascii_str_arr/test.rs b/src/ascii_str_arr/test.rs new file mode 100644 index 0000000..10385b4 --- /dev/null +++ b/src/ascii_str_arr/test.rs @@ -0,0 +1,52 @@ +//! Tests + +// Imports +use std::mem::MaybeUninit; + +use super::AsciiStrArr; +use ascii::{AsciiChar, AsciiStr}; + +#[test] +pub fn new() { + const N: usize = 10; + let mut ascii = AsciiStrArr::::new(); + assert_eq!(ascii, AsciiStrArr::default()); + + // SAFETY: We're initializing all elements + let mut cur_len = 0; + for c in unsafe { ascii.buffer_mut() } { + *c = MaybeUninit::new(AsciiChar::Null); + } + + loop { + assert_eq!(ascii.len(), cur_len); + assert_eq!(ascii.is_empty(), cur_len == 0); + assert_eq!(ascii.as_ascii().len(), cur_len); + assert_eq!(ascii.as_ascii_mut().len(), cur_len); + assert_eq!(ascii.as_ascii_slice().len(), cur_len); + assert_eq!(ascii.as_ascii_slice_mut().len(), cur_len); + assert_eq!(ascii.as_bytes().len(), cur_len); + assert_eq!(ascii.as_str().len(), cur_len); + assert_eq!(ascii.get(0).copied(), if cur_len == 0 { None } else { Some(AsciiChar::Null) }); + assert_eq!(ascii.get_mut(0).copied(), if cur_len == 0 { None } else { Some(AsciiChar::Null) }); + assert_eq!(AsRef::<[AsciiChar]>::as_ref(&ascii).len(), cur_len); + assert_eq!(AsMut::<[AsciiChar]>::as_mut(&mut ascii).len(), cur_len); + assert_eq!(AsRef::::as_ref(&ascii).len(), cur_len); + assert_eq!(AsMut::::as_mut(&mut ascii).len(), cur_len); + assert_eq!(AsRef::<[u8]>::as_ref(&ascii).len(), cur_len); + assert_eq!(AsRef::::as_ref(&ascii).len(), cur_len); + assert_eq!(ascii, ascii.clone()); + assert_eq!(format!("{}", ascii), format!("{}", ascii.as_ascii())); + assert_eq!(format!("{:?}", ascii), format!("{:?}", ascii.as_ascii())); + + if cur_len < N { + cur_len += 1; + // SAFETY: All elements are initialized at the beginning and `cur_len <= N` + unsafe { + ascii.set_len(cur_len); + } + } else { + break; + } + } +} diff --git a/src/io/address/real.rs b/src/io/address/real.rs index 60e4dc2..cdab9b6 100644 --- a/src/io/address/real.rs +++ b/src/io/address/real.rs @@ -14,7 +14,7 @@ use crate::{ #[derive(derive_more::From, derive_more::Into)] pub struct Real(u64); -/// Error type for [`Real::to_data`] +/// Error type for [`Real::try_to_data`] #[derive(PartialEq, Eq, Clone, Copy, Debug, thiserror::Error)] pub enum ToDataError { /// Occurs when the Real is outside of the data section of the sector diff --git a/src/lib.rs b/src/lib.rs index 83d0dae..09543d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -40,12 +40,19 @@ const_fn, const_panic, min_const_generics, - exclusive_range_pattern + exclusive_range_pattern, + unsafe_block_in_unsafe_fn, + maybe_uninit_uninit_array, + maybe_uninit_slice, + array_map )] // Lints #![warn(clippy::restriction, clippy::pedantic, clippy::nursery)] // Instead of `unwrap`, we must use `expect` and provide a reason #![forbid(clippy::unwrap_used)] +// We must use `unsafe` in unsafe `fn`s and specify if the guarantee is +// made by the caller or by us. +#![forbid(unsafe_op_in_unsafe_fn)] // We'll disable the ones we don't need #![allow(clippy::blanket_clippy_restriction_lints)] // Necessary items may be inlined using `LTO`, so we don't need to mark them as inline