From a270b6c254eab6acff2dab2acd07227e9e8b39b6 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Mon, 7 Feb 2022 00:15:09 +0000 Subject: [PATCH] Simplify implementation of arrays --- lib/src/strings.rs | 65 +++++++++++++++++++---------------- lib/src/task.rs | 4 +-- lib/src/traits.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++++ lib/src/util.rs | 22 +----------- lib/taskchampion.h | 7 ++-- 5 files changed, 130 insertions(+), 54 deletions(-) diff --git a/lib/src/strings.rs b/lib/src/strings.rs index 7d7e5bda3..55572ec25 100644 --- a/lib/src/strings.rs +++ b/lib/src/strings.rs @@ -1,6 +1,5 @@ use crate::string::TCString; use crate::traits::*; -use crate::util::{drop_pointer_array, vec_into_raw_parts}; use std::ptr::NonNull; /// TCStrings represents a list of strings. @@ -14,51 +13,59 @@ pub struct TCStrings { /// total size of items (internal use only) _capacity: libc::size_t, - /// strings representing each string. these remain owned by the TCStrings instance and will be freed - /// by tc_strings_free. + /// TCStrings representing each string. these remain owned by the TCStrings instance and will + /// be freed by tc_strings_free. This pointer is never NULL for a valid TCStrings, and the + /// *TCStrings at indexes 0..len-1 are not NULL. items: *const NonNull>, } -impl PassByValue for TCStrings { - type RustType = Vec>>; +impl PointerArray for TCStrings { + type Element = TCString<'static>; - unsafe fn from_ctype(self) -> Self::RustType { - // SAFETY: - // - C treats TCStrings as read-only, so items, len, and _capacity all came - // from a Vec originally. - unsafe { Vec::from_raw_parts(self.items as *mut _, self.len, self._capacity) } - } - - fn as_ctype(arg: Self::RustType) -> Self { - // emulate Vec::into_raw_parts(): - // - disable dropping the Vec with ManuallyDrop - // - extract ptr, len, and capacity using those methods - let (items, len, _capacity) = vec_into_raw_parts(arg); + unsafe fn from_raw_parts(items: *const NonNull, len: usize, cap: usize) -> Self { TCStrings { len, - _capacity, + _capacity: cap, items, } } -} -impl Default for TCStrings { - fn default() -> Self { - Self { - len: 0, - _capacity: 0, - items: std::ptr::null(), - } + fn into_raw_parts(self) -> (*const NonNull, usize, usize) { + (self.items, self.len, self._capacity) } } /// Free a TCStrings instance. The instance, and all TCStrings it contains, must not be used after /// this call. +/// +/// When this call returns, the `items` pointer will be NULL, signalling an invalid TCStrings. #[no_mangle] -pub extern "C" fn tc_strings_free<'a>(tcstrings: *mut TCStrings) { +pub extern "C" fn tc_strings_free(tcstrings: *mut TCStrings) { debug_assert!(!tcstrings.is_null()); // SAFETY: // - *tcstrings is a valid TCStrings (caller promises to treat it as read-only) - let strings = unsafe { TCStrings::take_from_arg(tcstrings, TCStrings::default()) }; - drop_pointer_array(strings); + let strings = unsafe { TCStrings::take_from_arg(tcstrings, TCStrings::null_value()) }; + TCStrings::drop_pointer_vector(strings); +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn empty_array_has_non_null_pointer() { + let tcstrings = TCStrings::return_val(Vec::new()); + assert!(!tcstrings.items.is_null()); + assert_eq!(tcstrings.len, 0); + assert_eq!(tcstrings._capacity, 0); + } + + #[test] + fn free_sets_null_pointer() { + let mut tcstrings = TCStrings::return_val(Vec::new()); + tc_strings_free(&mut tcstrings); + assert!(tcstrings.items.is_null()); + assert_eq!(tcstrings.len, 0); + assert_eq!(tcstrings._capacity, 0); + } } diff --git a/lib/src/task.rs b/lib/src/task.rs index 1a2bbaf6d..823fbbffe 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -321,7 +321,7 @@ pub extern "C" fn tc_task_has_tag<'a>(task: *mut TCTask, tag: *mut TCString) -> #[no_mangle] pub extern "C" fn tc_task_get_tags<'a>(task: *mut TCTask) -> TCStrings { wrap(task, |task| { - let tcstrings: Vec>> = task + let vec: Vec>> = task .get_tags() .map(|t| { NonNull::new( @@ -331,7 +331,7 @@ pub extern "C" fn tc_task_get_tags<'a>(task: *mut TCTask) -> TCStrings { .expect("TCString::return_val() returned NULL") }) .collect(); - TCStrings::return_val(tcstrings) + TCStrings::return_val(vec) }) } diff --git a/lib/src/traits.rs b/lib/src/traits.rs index df57d11c7..1e4a67dc6 100644 --- a/lib/src/traits.rs +++ b/lib/src/traits.rs @@ -1,3 +1,6 @@ +use crate::util::vec_into_raw_parts; +use std::ptr::NonNull; + /// Support for values passed to Rust by value. These are represented as full structs in C. Such /// values are implicitly copyable, via C's struct assignment. /// @@ -142,3 +145,86 @@ pub(crate) trait PassByPointer: Sized { } } } + +/// Support for arrays of objects referenced by pointer. +/// +/// The underlying C type should have three fields, containing items, length, and capacity. The +/// required trait functions just fetch and set these fields. The PassByValue trait will be +/// implemented automatically, converting between the C type and `Vec>`. For most +/// cases, it is only necessary to implement `tc_.._free` that first calls +/// `PassByValue::take_from_arg(arg, PointerArray::null_value())` to take the existing value and +/// replace it with the null value; then `PointerArray::drop_pointer_vector(..)` to drop the +/// resulting vector and all of the objects it points to. +/// +/// # Safety +/// +/// The C type must be documented as read-only. None of the fields may be modified, nor anything +/// in the `items` array. +/// +/// This class guarantees that the items pointer is non-NULL for any valid array (even when len=0), +/// and that all pointers at indexes 0..len-1 are non-NULL. +pub(crate) trait PointerArray: Sized { + type Element: 'static + PassByPointer; + + /// Create a new PointerArray from the given items, len, and capacity. + /// + /// # Safety + /// + /// The arguments must either: + /// - be NULL, 0, and 0, respectively; or + /// - be valid for Vec::from_raw_parts + unsafe fn from_raw_parts(items: *const NonNull, len: usize, cap: usize) -> Self; + + /// Get the items, len, and capacity (in that order) for this instance. These must be + /// precisely the same values passed tearlier to `from_raw_parts`. + fn into_raw_parts(self) -> (*const NonNull, usize, usize); + + /// Generate a NULL value. By default this is a NULL items pointer with zero length and + /// capacity. + fn null_value() -> Self { + // SAFETY: + // - satisfies the first case in from_raw_parts' safety documentation + unsafe { Self::from_raw_parts(std::ptr::null(), 0, 0) } + } + + /// Drop a vector of element pointers. This is a convenience function for implementing + /// tc_.._free functions. + fn drop_pointer_vector(mut vec: Vec>) { + // first, drop each of the elements in turn + for p in vec.drain(..) { + // SAFETY: + // - p is not NULL (NonNull) + // - p was generated by Rust (true for all arrays) + // - p was not modified (all arrays are immutable from C) + // - caller will not use this pointer again (promised by caller; p has been drain'd from + // the vector) + drop(unsafe { PassByPointer::take_from_arg(p.as_ptr()) }); + } + drop(vec); + } +} + +impl PassByValue for A +where + A: PointerArray, +{ + type RustType = Vec>; + + unsafe fn from_ctype(self) -> Self::RustType { + let (items, len, cap) = self.into_raw_parts(); + debug_assert!(!items.is_null()); + // SAFETY: + // - PointerArray::from_raw_parts requires that items, len, and cap be valid for + // Vec::from_raw_parts if not NULL, and they are not NULL (as promised by caller) + // - PointerArray::into_raw_parts returns precisely the values passed to from_raw_parts. + // - those parts are passed to Vec::from_raw_parts here. + unsafe { Vec::from_raw_parts(items as *mut _, len, cap) } + } + + fn as_ctype(arg: Self::RustType) -> Self { + let (items, len, cap) = vec_into_raw_parts(arg); + // SAFETY: + // - satisfies the second case in from_raw_parts' safety documentation + unsafe { Self::from_raw_parts(items, len, cap) } + } +} diff --git a/lib/src/util.rs b/lib/src/util.rs index 00676424a..bcab209ec 100644 --- a/lib/src/util.rs +++ b/lib/src/util.rs @@ -1,12 +1,10 @@ use crate::string::TCString; -use crate::traits::*; -use std::ptr::NonNull; pub(crate) fn err_to_tcstring(e: impl std::string::ToString) -> TCString<'static> { TCString::from(e.to_string()) } -/// A version of Vec::into_raw_parts, which is still unstable. Returns ptr, len, cap. +/// An implementation of Vec::into_raw_parts, which is still unstable. Returns ptr, len, cap. pub(crate) fn vec_into_raw_parts(vec: Vec) -> (*mut T, usize, usize) { // emulate Vec::into_raw_parts(): // - disable dropping the Vec with ManuallyDrop @@ -14,21 +12,3 @@ pub(crate) fn vec_into_raw_parts(vec: Vec) -> (*mut T, usize, usize) { let mut vec = std::mem::ManuallyDrop::new(vec); return (vec.as_mut_ptr(), vec.len(), vec.capacity()); } - -/// Drop an array of PassByPointer values -pub(crate) fn drop_pointer_array(mut array: Vec>) -where - T: 'static + PassByPointer, -{ - // first, drop each of the elements in turn - for p in array.drain(..) { - // SAFETY: - // - p is not NULL (NonNull) - // - p was generated by Rust (true for all arrays) - // - p was not modified (all arrays are immutable from C) - // - caller will not use this pointer again (promised by caller; p has been drain'd from - // the vector) - drop(unsafe { PassByPointer::take_from_arg(p.as_ptr()) }); - } - drop(array); -} diff --git a/lib/taskchampion.h b/lib/taskchampion.h index 818fc166f..86aa40cba 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -141,8 +141,9 @@ typedef struct TCStrings { */ size_t _capacity; /** - * strings representing each string. these remain owned by the TCStrings instance and will be freed - * by tc_strings_free. + * TCStrings representing each string. these remain owned by the TCStrings instance and will + * be freed by tc_strings_free. This pointer is never NULL for a valid TCStrings, and the + * *TCStrings at indexes 0..len-1 are not NULL. */ struct TCString *const *items; } TCStrings; @@ -272,6 +273,8 @@ void tc_string_free(struct TCString *tcstring); /** * Free a TCStrings instance. The instance, and all TCStrings it contains, must not be used after * this call. + * + * When this call returns, the `items` pointer will be NULL, signalling an invalid TCStrings. */ void tc_strings_free(struct TCStrings *tcstrings);