diff --git a/integration-tests/src/bindings_tests/task.c b/integration-tests/src/bindings_tests/task.c index f3ff29ae0..4e20e52b3 100644 --- a/integration-tests/src/bindings_tests/task.c +++ b/integration-tests/src/bindings_tests/task.c @@ -561,6 +561,53 @@ static void test_task_taskmap(void) { tc_replica_free(rep); } +// taking from a task list behaves correctly +static void test_task_list_take(void) { + TCReplica *rep = tc_replica_new_in_memory(); + TEST_ASSERT_NULL(tc_replica_error(rep).ptr); + + TCTask *task1 = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("t")); + TEST_ASSERT_NOT_NULL(task1); + + TCTask *task2 = tc_replica_new_task(rep, TC_STATUS_PENDING, tc_string_borrow("t")); + TEST_ASSERT_NOT_NULL(task2); + tc_task_free(task2); + + TCString desc; + TCTaskList tasks = tc_replica_all_tasks(rep); + TEST_ASSERT_NOT_NULL(tasks.items); + TEST_ASSERT_EQUAL(2, tasks.len); + + task1 = tc_task_list_take(&tasks, 5); // out of bounds + TEST_ASSERT_NULL(task1); + + task1 = tc_task_list_take(&tasks, 0); + TEST_ASSERT_NOT_NULL(task1); + desc = tc_task_get_description(task1); + TEST_ASSERT_EQUAL_STRING("t", tc_string_content(&desc)); + tc_string_free(&desc); + + task2 = tc_task_list_take(&tasks, 1); + TEST_ASSERT_NOT_NULL(task2); + desc = tc_task_get_description(task2); + TEST_ASSERT_EQUAL_STRING("t", tc_string_content(&desc)); + tc_string_free(&desc); + + tc_task_free(task1); + tc_task_free(task2); + + task1 = tc_task_list_take(&tasks, 0); // already taken + TEST_ASSERT_NULL(task1); + + task1 = tc_task_list_take(&tasks, 5); // out of bounds + TEST_ASSERT_NULL(task1); + + tc_task_list_free(&tasks); + TEST_ASSERT_NULL(tasks.items); + + tc_replica_free(rep); +} + int task_tests(void) { UNITY_BEGIN(); // each test case above should be named here, in order. @@ -578,5 +625,6 @@ int task_tests(void) { RUN_TEST(test_task_annotations); RUN_TEST(test_task_udas); RUN_TEST(test_task_taskmap); + RUN_TEST(test_task_list_take); return UNITY_END(); } diff --git a/lib/src/annotation.rs b/lib/src/annotation.rs index 774f94478..06c3069b2 100644 --- a/lib/src/annotation.rs +++ b/lib/src/annotation.rs @@ -75,13 +75,13 @@ pub struct TCAnnotationList { /// array of annotations. these remain owned by the TCAnnotationList instance and will be freed by /// tc_annotation_list_free. This pointer is never NULL for a valid TCAnnotationList. - items: *const TCAnnotation, + items: *mut TCAnnotation, } impl CList for TCAnnotationList { type Element = TCAnnotation; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCAnnotationList { len, _capacity: cap, @@ -89,7 +89,16 @@ impl CList for TCAnnotationList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } diff --git a/lib/src/kv.rs b/lib/src/kv.rs index 716883c75..8fcbaabbe 100644 --- a/lib/src/kv.rs +++ b/lib/src/kv.rs @@ -50,13 +50,13 @@ pub struct TCKVList { /// array of TCKV's. these remain owned by the TCKVList instance and will be freed by /// tc_kv_list_free. This pointer is never NULL for a valid TCKVList. - items: *const TCKV, + items: *mut TCKV, } impl CList for TCKVList { type Element = TCKV; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCKVList { len, _capacity: cap, @@ -64,7 +64,16 @@ impl CList for TCKVList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } diff --git a/lib/src/replica.rs b/lib/src/replica.rs index cec26ad42..6d9683959 100644 --- a/lib/src/replica.rs +++ b/lib/src/replica.rs @@ -177,12 +177,14 @@ pub unsafe extern "C" fn tc_replica_all_tasks(rep: *mut TCReplica) -> TCTaskList .all_tasks()? .drain() .map(|(_uuid, t)| { - NonNull::new( - // SAFETY: - // - caller promises to free this value (via freeing the list) - unsafe { TCTask::from(t).return_ptr() }, + Some( + NonNull::new( + // SAFETY: + // - caller promises to free this value (via freeing the list) + unsafe { TCTask::from(t).return_ptr() }, + ) + .expect("TCTask::return_ptr returned NULL"), ) - .expect("TCTask::return_ptr returned NULL") }) .collect(); // SAFETY: diff --git a/lib/src/string.rs b/lib/src/string.rs index 719a30563..65c9dfd9e 100644 --- a/lib/src/string.rs +++ b/lib/src/string.rs @@ -374,13 +374,13 @@ pub struct TCStringList { /// TCStringList representing each string. these remain owned by the TCStringList instance and will /// be freed by tc_string_list_free. This pointer is never NULL for a valid TCStringList, and the /// *TCStringList at indexes 0..len-1 are not NULL. - items: *const TCString, + items: *mut TCString, } impl CList for TCStringList { type Element = TCString; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCStringList { len, _capacity: cap, @@ -388,7 +388,16 @@ impl CList for TCStringList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } diff --git a/lib/src/task.rs b/lib/src/task.rs index 159553760..8bcdde446 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -171,7 +171,10 @@ impl TryFrom> for Tag { /// TCTaskList represents a list of tasks. /// -/// The content of this struct must be treated as read-only. +/// The content of this struct must be treated as read-only: no fields or anything they reference +/// should be modified directly by C code. +/// +/// When an item is taken from this list, its pointer in `items` is set to NULL. #[repr(C)] pub struct TCTaskList { /// number of tasks in items @@ -183,13 +186,13 @@ pub struct TCTaskList { /// array of pointers representing each task. these remain owned by the TCTaskList instance and /// will be freed by tc_task_list_free. This pointer is never NULL for a valid TCTaskList, /// and the *TCTaskList at indexes 0..len-1 are not NULL. - items: *const NonNull, + items: *mut Option>, } impl CList for TCTaskList { - type Element = NonNull; + type Element = Option>; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCTaskList { len, _capacity: cap, @@ -197,7 +200,16 @@ impl CList for TCTaskList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } @@ -813,17 +825,39 @@ pub unsafe extern "C" fn tc_task_free(task: *mut TCTask) { drop(tctask); } +/// Take an item from a TCTaskList. After this call, the indexed item is no longer associated +/// with the list and becomes the caller's responsibility, just as if it had been returned from +/// `tc_replica_get_task`. +/// +/// The corresponding element in the `items` array will be set to NULL. If that field is already +/// NULL (that is, if the item has already been taken), this function will return NULL. If the +/// index is out of bounds, this function will also return NULL. +/// +/// The passed TCTaskList remains owned by the caller. +#[no_mangle] +pub unsafe extern "C" fn tc_task_list_take(tasks: *mut TCTaskList, index: usize) -> *mut TCTask { + // SAFETY: + // - tasks is not NULL and points to a valid TCTaskList (caller is not allowed to + // modify the list directly, and tc_task_list_take leaves the list valid) + let p = unsafe { take_optional_pointer_list_item(tasks, index) }; + if let Some(p) = p { + p.as_ptr() + } else { + std::ptr::null_mut() + } +} + /// Free a TCTaskList instance. The instance, and all TCTaskList it contains, must not be used after /// this call. /// /// When this call returns, the `items` pointer will be NULL, signalling an invalid TCTaskList. #[no_mangle] -pub unsafe extern "C" fn tc_task_list_free(tctasks: *mut TCTaskList) { +pub unsafe extern "C" fn tc_task_list_free(tasks: *mut TCTaskList) { // SAFETY: - // - tctasks is not NULL and points to a valid TCTaskList (caller is not allowed to - // modify the list) + // - tasks is not NULL and points to a valid TCTaskList (caller is not allowed to + // modify the list directly, and tc_task_list_take leaves the list valid) // - caller promises not to use the value after return - unsafe { drop_pointer_list(tctasks) }; + unsafe { drop_optional_pointer_list(tasks) }; } #[cfg(test)] @@ -832,19 +866,19 @@ mod test { #[test] fn empty_list_has_non_null_pointer() { - let tctasks = unsafe { TCTaskList::return_val(Vec::new()) }; - assert!(!tctasks.items.is_null()); - assert_eq!(tctasks.len, 0); - assert_eq!(tctasks._capacity, 0); + let tasks = unsafe { TCTaskList::return_val(Vec::new()) }; + assert!(!tasks.items.is_null()); + assert_eq!(tasks.len, 0); + assert_eq!(tasks._capacity, 0); } #[test] fn free_sets_null_pointer() { - let mut tctasks = unsafe { TCTaskList::return_val(Vec::new()) }; + let mut tasks = unsafe { TCTaskList::return_val(Vec::new()) }; // SAFETY: testing expected behavior - unsafe { tc_task_list_free(&mut tctasks) }; - assert!(tctasks.items.is_null()); - assert_eq!(tctasks.len, 0); - assert_eq!(tctasks._capacity, 0); + unsafe { tc_task_list_free(&mut tasks) }; + assert!(tasks.items.is_null()); + assert_eq!(tasks.len, 0); + assert_eq!(tasks._capacity, 0); } } diff --git a/lib/src/traits.rs b/lib/src/traits.rs index ffbdf9a79..5322eccb0 100644 --- a/lib/src/traits.rs +++ b/lib/src/traits.rs @@ -150,13 +150,17 @@ pub(crate) trait PassByPointer: Sized { /// 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 calls either -/// drop_value_list (if Element is PassByValue) or drop_pointer_list (if element is PassByPointer). +/// The element type can be PassByValue or PassByPointer. If the latter, it should use either +/// `NonNull` or `Option>` to represent the element. The latter is an "optional +/// pointer list", where elements can be omitted. +/// +/// For most cases, it is only necessary to implement `tc_.._free` that calls one of the +/// drop_..._list functions. /// /// # Safety /// /// The C type must be documented as read-only. None of the fields may be modified, nor anything -/// accessible via the `items` array. +/// accessible via the `items` array. The exception is modification via "taking" elements. /// /// This class guarantees that the items pointer is non-NULL for any valid list (even when len=0). pub(crate) trait CList: Sized { @@ -169,18 +173,21 @@ pub(crate) trait CList: Sized { /// 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 Self::Element, len: usize, cap: usize) -> Self; + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self; + + /// Return a mutable slice representing the elements in this list. + fn slice(&mut self) -> &mut [Self::Element]; /// 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 Self::Element, usize, usize); + fn into_raw_parts(self) -> (*mut 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) } + unsafe { Self::from_raw_parts(std::ptr::null_mut(), 0, 0) } } } @@ -225,6 +232,7 @@ where /// - List must be non-NULL and point to a valid CL instance /// - The caller must not use the value array points to after this function, as /// it has been freed. It will be replaced with the null value. +#[allow(dead_code)] // this was useful once, and might be again? pub(crate) unsafe fn drop_pointer_list(list: *mut CL) where CL: CList>, @@ -246,6 +254,79 @@ where drop(vec); } +/// Given a CList containing optional pointers, drop all of the non-null pointed-to values and the +/// list. +/// +/// This is a convenience function for `tc_.._list_free` functions, for lists from which items +/// can be taken. +/// +/// # Safety +/// +/// - List must be non-NULL and point to a valid CL instance +/// - The caller must not use the value array points to after this function, as +/// it has been freed. It will be replaced with the null value. +pub(crate) unsafe fn drop_optional_pointer_list(list: *mut CL) +where + CL: CList>>, + T: PassByPointer, +{ + debug_assert!(!list.is_null()); + // SAFETY: + // - *list is a valid CL (promised by caller) + let mut vec = unsafe { CL::take_val_from_arg(list, CL::null_value()) }; + + // first, drop each of the elements in turn + for e in vec.drain(..) { + if let Some(e) = e { + // SAFETY: + // - e is a valid Element (promised by caller) + // - e is owned + drop(unsafe { PassByPointer::take_from_ptr_arg(e.as_ptr()) }); + } + } + // then drop the vector + drop(vec); +} + +/// Take a value from an optional pointer list, returning the value and replacing its array +/// element with NULL. +/// +/// This is a convenience function for `tc_.._list_take` functions, for lists from which items +/// can be taken. +/// +/// The returned value will be None if the element has already been taken, or if the index is +/// out of bounds. +/// +/// # Safety +/// +/// - List must be non-NULL and point to a valid CL instance +pub(crate) unsafe fn take_optional_pointer_list_item( + list: *mut CL, + index: usize, +) -> Option> +where + CL: CList>>, + T: PassByPointer, +{ + debug_assert!(!list.is_null()); + + // SAFETy: + // - list is properly aligned, dereferencable, and points to an initialized CL, since it is valid + // - the lifetime of the resulting reference is limited to this function, during which time + // nothing else refers to this memory. + let slice = list.as_mut().unwrap().slice(); + if let Some(elt_ref) = slice.get_mut(index) { + let mut rv = None; + if let Some(elt) = elt_ref.as_mut() { + rv = Some(*elt); + *elt_ref = None; // clear out the array element + } + rv + } else { + None // index out of bounds + } +} + impl PassByValue for A where A: CList, diff --git a/lib/src/uda.rs b/lib/src/uda.rs index 30607090c..679557504 100644 --- a/lib/src/uda.rs +++ b/lib/src/uda.rs @@ -72,13 +72,13 @@ pub struct TCUdaList { /// array of UDAs. These remain owned by the TCUdaList instance and will be freed by /// tc_uda_list_free. This pointer is never NULL for a valid TCUdaList. - items: *const TCUda, + items: *mut TCUda, } impl CList for TCUdaList { type Element = TCUda; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCUdaList { len, _capacity: cap, @@ -86,7 +86,16 @@ impl CList for TCUdaList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } diff --git a/lib/src/uuid.rs b/lib/src/uuid.rs index 28421a81c..8284caac2 100644 --- a/lib/src/uuid.rs +++ b/lib/src/uuid.rs @@ -57,13 +57,13 @@ pub struct TCUuidList { /// array of uuids. these remain owned by the TCUuidList instance and will be freed by /// tc_uuid_list_free. This pointer is never NULL for a valid TCUuidList. - items: *const TCUuid, + items: *mut TCUuid, } impl CList for TCUuidList { type Element = TCUuid; - unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *mut Self::Element, len: usize, cap: usize) -> Self { TCUuidList { len, _capacity: cap, @@ -71,7 +71,16 @@ impl CList for TCUuidList { } } - fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + fn slice(&mut self) -> &mut [Self::Element] { + // SAFETY: + // - because we have &mut self, we have read/write access to items[0..len] + // - all items are properly initialized Element's + // - return value lifetime is equal to &mmut self's, so access is exclusive + // - items and len came from Vec, so total size is < isize::MAX + unsafe { std::slice::from_raw_parts_mut(self.items, self.len) } + } + + fn into_raw_parts(self) -> (*mut Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } diff --git a/lib/taskchampion.h b/lib/taskchampion.h index f09bddfe1..5e9c728bb 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -301,7 +301,7 @@ typedef struct TCAnnotationList { * array of annotations. these remain owned by the TCAnnotationList instance and will be freed by * tc_annotation_list_free. This pointer is never NULL for a valid TCAnnotationList. */ - const struct TCAnnotation *items; + struct TCAnnotation *items; } TCAnnotationList; /** @@ -333,13 +333,16 @@ typedef struct TCKVList { * array of TCKV's. these remain owned by the TCKVList instance and will be freed by * tc_kv_list_free. This pointer is never NULL for a valid TCKVList. */ - const struct TCKV *items; + struct TCKV *items; } TCKVList; /** * TCTaskList represents a list of tasks. * - * The content of this struct must be treated as read-only. + * The content of this struct must be treated as read-only: no fields or anything they reference + * should be modified directly by C code. + * + * When an item is taken from this list, its pointer in `items` is set to NULL. */ typedef struct TCTaskList { /** @@ -355,7 +358,7 @@ typedef struct TCTaskList { * will be freed by tc_task_list_free. This pointer is never NULL for a valid TCTaskList, * and the *TCTaskList at indexes 0..len-1 are not NULL. */ - struct TCTask *const *items; + struct TCTask **items; } TCTaskList; /** @@ -385,7 +388,7 @@ typedef struct TCUuidList { * array of uuids. these remain owned by the TCUuidList instance and will be freed by * tc_uuid_list_free. This pointer is never NULL for a valid TCUuidList. */ - const struct TCUuid *items; + struct TCUuid *items; } TCUuidList; /** @@ -407,7 +410,7 @@ typedef struct TCStringList { * be freed by tc_string_list_free. This pointer is never NULL for a valid TCStringList, and the * *TCStringList at indexes 0..len-1 are not NULL. */ - const struct TCString *items; + struct TCString *items; } TCStringList; /** @@ -446,7 +449,7 @@ typedef struct TCUdaList { * array of UDAs. These remain owned by the TCUdaList instance and will be freed by * tc_uda_list_free. This pointer is never NULL for a valid TCUdaList. */ - const struct TCUda *items; + struct TCUda *items; } TCUdaList; #ifdef __cplusplus @@ -920,13 +923,26 @@ struct TCString tc_task_error(struct TCTask *task); */ void tc_task_free(struct TCTask *task); +/** + * Take an item from a TCTaskList. After this call, the indexed item is no longer associated + * with the list and becomes the caller's responsibility, just as if it had been returned from + * `tc_replica_get_task`. + * + * The corresponding element in the `items` array will be set to NULL. If that field is already + * NULL (that is, if the item has already been taken), this function will return NULL. If the + * index is out of bounds, this function will also return NULL. + * + * The passed TCTaskList remains owned by the caller. + */ +struct TCTask *tc_task_list_take(struct TCTaskList *tasks, size_t index); + /** * Free a TCTaskList instance. The instance, and all TCTaskList it contains, must not be used after * this call. * * When this call returns, the `items` pointer will be NULL, signalling an invalid TCTaskList. */ -void tc_task_list_free(struct TCTaskList *tctasks); +void tc_task_list_free(struct TCTaskList *tasks); /** * Free a TCUda instance. The instance, and the TCStrings it contains, must not be used