Simplify implementation of arrays

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<TCString<'static>>,
 }
 
-impl PassByValue for TCStrings {
-    type RustType = Vec<NonNull<TCString<'static>>>;
+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<Self::Element>, 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<Self::Element>, 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);
+    }
 }

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<NonNull<TCString<'static>>> = task
+        let vec: Vec<NonNull<TCString<'static>>> = 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)
     })
 }
 

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<NonNull<Element>>`.  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<Self::Element>, 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<Self::Element>, 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<NonNull<Self::Element>>) {
+        // 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<A> PassByValue for A
+where
+    A: PointerArray,
+{
+    type RustType = Vec<NonNull<A::Element>>;
+
+    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) }
+    }
+}

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<T>(vec: Vec<T>) -> (*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<T>(vec: Vec<T>) -> (*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<T>(mut array: Vec<NonNull<T>>)
-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);
-}

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);