From a4d992012e469f6e917f19bacc96c6638da369c0 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Thu, 10 Feb 2022 00:55:34 +0000 Subject: [PATCH] TCUuidList, refactor traits --- .../src/bindings_tests/replica.c | 73 ++++++++---- lib/src/lib.rs | 1 + lib/src/replica.rs | 22 +++- lib/src/stringlist.rs | 10 +- lib/src/tasklist.rs | 10 +- lib/src/traits.rs | 112 +++++++++++++----- lib/src/uuidlist.rs | 70 +++++++++++ lib/taskchampion.h | 36 ++++++ 8 files changed, 271 insertions(+), 63 deletions(-) create mode 100644 lib/src/uuidlist.rs diff --git a/integration-tests/src/bindings_tests/replica.c b/integration-tests/src/bindings_tests/replica.c index 41cf2dfe8..14084ab00 100644 --- a/integration-tests/src/bindings_tests/replica.c +++ b/integration-tests/src/bindings_tests/replica.c @@ -73,45 +73,70 @@ static void test_replica_task_creation(void) { tc_replica_free(rep); } -// a replica with tasks in it returns an appropriate list of tasks +// a replica with tasks in it returns an appropriate list of tasks and list of uuids static void test_replica_all_tasks(void) { TCReplica *rep = tc_replica_new_in_memory(); TEST_ASSERT_NULL(tc_replica_error(rep)); - TCTask *task = tc_replica_new_task( + TCTask *task1 = tc_replica_new_task( rep, TC_STATUS_PENDING, tc_string_borrow("task1")); - TEST_ASSERT_NOT_NULL(task); - tc_task_free(task); + TEST_ASSERT_NOT_NULL(task1); + TCUuid uuid1 = tc_task_get_uuid(task1); + tc_task_free(task1); - task = tc_replica_new_task( + TCTask *task2 = tc_replica_new_task( rep, TC_STATUS_PENDING, tc_string_borrow("task2")); - TEST_ASSERT_NOT_NULL(task); - tc_task_free(task); + TEST_ASSERT_NOT_NULL(task2); + TCUuid uuid2 = tc_task_get_uuid(task2); + tc_task_free(task2); - TCTaskList tasks = tc_replica_all_tasks(rep); - TEST_ASSERT_NOT_NULL(tasks.items); - TEST_ASSERT_EQUAL(2, tasks.len); + { + TCTaskList tasks = tc_replica_all_tasks(rep); + TEST_ASSERT_NOT_NULL(tasks.items); + TEST_ASSERT_EQUAL(2, tasks.len); - int seen1, seen2 = false; - for (size_t i = 0; i < tasks.len; i++) { - TCTask *task = tasks.items[i]; - TCString *descr = tc_task_get_description(task); - if (0 == strcmp(tc_string_content(descr), "task1")) { - seen1 = true; - } else if (0 == strcmp(tc_string_content(descr), "task2")) { - seen2 = true; + bool seen1, seen2 = false; + for (size_t i = 0; i < tasks.len; i++) { + TCTask *task = tasks.items[i]; + TCString *descr = tc_task_get_description(task); + if (0 == strcmp(tc_string_content(descr), "task1")) { + seen1 = true; + } else if (0 == strcmp(tc_string_content(descr), "task2")) { + seen2 = true; + } + tc_string_free(descr); } - tc_string_free(descr); - } - TEST_ASSERT_TRUE(seen1); - TEST_ASSERT_TRUE(seen2); + TEST_ASSERT_TRUE(seen1); + TEST_ASSERT_TRUE(seen2); - tc_task_list_free(&tasks); - TEST_ASSERT_NULL(tasks.items); + tc_task_list_free(&tasks); + TEST_ASSERT_NULL(tasks.items); + } + + { + TCUuidList uuids = tc_replica_all_task_uuids(rep); + TEST_ASSERT_NOT_NULL(uuids.items); + TEST_ASSERT_EQUAL(2, uuids.len); + + bool seen1, seen2 = false; + for (size_t i = 0; i < uuids.len; i++) { + TCUuid uuid = uuids.items[i]; + if (0 == memcmp(&uuid1, &uuid, sizeof(TCUuid))) { + seen1 = true; + } else if (0 == memcmp(&uuid2, &uuid, sizeof(TCUuid))) { + seen2 = true; + } + } + TEST_ASSERT_TRUE(seen1); + TEST_ASSERT_TRUE(seen2); + + tc_uuid_list_free(&uuids); + TEST_ASSERT_NULL(uuids.items); + } tc_replica_free(rep); } diff --git a/lib/src/lib.rs b/lib/src/lib.rs index f9e353d21..bb4ebe905 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -14,3 +14,4 @@ pub mod stringlist; pub mod task; pub mod tasklist; pub mod uuid; +pub mod uuidlist; diff --git a/lib/src/replica.rs b/lib/src/replica.rs index b17c256a1..5119ea520 100644 --- a/lib/src/replica.rs +++ b/lib/src/replica.rs @@ -2,7 +2,7 @@ use crate::traits::*; use crate::util::err_to_tcstring; use crate::{ result::TCResult, status::TCStatus, string::TCString, task::TCTask, tasklist::TCTaskList, - uuid::TCUuid, + uuid::TCUuid, uuidlist::TCUuidList, }; use std::ptr::NonNull; use taskchampion::{Replica, StorageConfig}; @@ -161,7 +161,25 @@ pub unsafe extern "C" fn tc_replica_all_tasks(rep: *mut TCReplica) -> TCTaskList ) } -// TODO: tc_replica_all_task_uuids +/// Get a list of all uuids for tasks in the replica. +/// +/// Returns a TCUuidList with a NULL items field on error. +#[no_mangle] +pub unsafe extern "C" fn tc_replica_all_task_uuids(rep: *mut TCReplica) -> TCUuidList { + wrap( + rep, + |rep| { + let uuids: Vec<_> = rep + .all_task_uuids()? + .drain(..) + .map(|uuid| TCUuid::return_val(uuid)) + .collect(); + Ok(TCUuidList::return_val(uuids)) + }, + TCUuidList::null_value(), + ) +} + // TODO: tc_replica_working_set /// Get an existing task by its UUID. diff --git a/lib/src/stringlist.rs b/lib/src/stringlist.rs index 3daf5f3cd..3ee3be48e 100644 --- a/lib/src/stringlist.rs +++ b/lib/src/stringlist.rs @@ -19,10 +19,10 @@ pub struct TCStringList { items: *const NonNull>, } -impl PointerArray for TCStringList { - type Element = TCString<'static>; +impl ValueArray for TCStringList { + type Element = NonNull>; - unsafe fn from_raw_parts(items: *const NonNull, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { TCStringList { len, _capacity: cap, @@ -30,7 +30,7 @@ impl PointerArray for TCStringList { } } - fn into_raw_parts(self) -> (*const NonNull, usize, usize) { + fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } @@ -45,7 +45,7 @@ pub unsafe extern "C" fn tc_string_list_free(tcstrings: *mut TCStringList) { // SAFETY: // - *tcstrings is a valid TCStringList (caller promises to treat it as read-only) let strings = unsafe { TCStringList::take_from_arg(tcstrings, TCStringList::null_value()) }; - TCStringList::drop_pointer_vector(strings); + TCStringList::drop_vector(strings); } #[cfg(test)] diff --git a/lib/src/tasklist.rs b/lib/src/tasklist.rs index eaba75ca1..c383139fe 100644 --- a/lib/src/tasklist.rs +++ b/lib/src/tasklist.rs @@ -19,10 +19,10 @@ pub struct TCTaskList { items: *const NonNull, } -impl PointerArray for TCTaskList { - type Element = TCTask; +impl ValueArray for TCTaskList { + type Element = NonNull; - unsafe fn from_raw_parts(items: *const NonNull, len: usize, cap: usize) -> Self { + unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { TCTaskList { len, _capacity: cap, @@ -30,7 +30,7 @@ impl PointerArray for TCTaskList { } } - fn into_raw_parts(self) -> (*const NonNull, usize, usize) { + fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { (self.items, self.len, self._capacity) } } @@ -45,7 +45,7 @@ pub unsafe extern "C" fn tc_task_list_free(tctasks: *mut TCTaskList) { // SAFETY: // - *tctasks is a valid TCTaskList (caller promises to treat it as read-only) let tasks = unsafe { TCTaskList::take_from_arg(tctasks, TCTaskList::null_value()) }; - TCTaskList::drop_pointer_vector(tasks); + TCTaskList::drop_vector(tasks); } #[cfg(test)] diff --git a/lib/src/traits.rs b/lib/src/traits.rs index 1e4a67dc6..f0bde9e33 100644 --- a/lib/src/traits.rs +++ b/lib/src/traits.rs @@ -146,38 +146,100 @@ pub(crate) trait PassByPointer: Sized { } } -/// Support for arrays of objects referenced by pointer. +/// *mut P can be passed by value +impl

PassByValue for *mut P +where + P: PassByPointer + 'static, +{ + type RustType = &'static mut P; + + unsafe fn from_ctype(self) -> Self::RustType { + // SAFETY: + // - self must be a valid *mut P (promised by caller) + // - TODO for 'static + unsafe { &mut *self } + } + + /// Convert a Rust value to a C value. + fn as_ctype(arg: Self::RustType) -> Self { + arg + } +} + +/// *const P can be passed by value +impl

PassByValue for *const P +where + P: PassByPointer + 'static, +{ + type RustType = &'static P; + + unsafe fn from_ctype(self) -> Self::RustType { + // SAFETY: + // - self must be a valid *mut P (promised by caller) + // - TODO for 'static + unsafe { &*self } + } + + /// Convert a Rust value to a C value. + fn as_ctype(arg: Self::RustType) -> Self { + arg + } +} + +/// NonNull

can be passed by value +impl

PassByValue for NonNull

+where + P: PassByPointer + 'static, +{ + type RustType = &'static mut P; + + unsafe fn from_ctype(mut self) -> Self::RustType { + // SAFETY: + // - self must be a valid *mut P (promised by caller) + // - TODO for 'static + unsafe { self.as_mut() } + } + + /// Convert a Rust value to a C value. + fn as_ctype(arg: Self::RustType) -> Self { + NonNull::new(arg).expect("value must not be NULL") + } +} + +/// Support for arrays of objects referenced by value. /// /// 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. +/// 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, ValueArray::null_value())` to take the existing value and +/// replace it with the null value; then `ValueArray::drop_value_vector(..)` to drop the resulting +/// vector and all of the objects it points to. +/// +/// This can be used for objects referenced by pointer, too, with an Element type of `*const T` /// /// # 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; +/// This class guarantees that the items pointer is non-NULL for any valid array (even when len=0). +// TODO: rename Array +pub(crate) trait ValueArray: Sized { + type Element: PassByValue; - /// Create a new PointerArray from the given items, len, and capacity. + /// Create a new ValueArray 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; + unsafe fn from_raw_parts(items: *const 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, usize, usize); + fn into_raw_parts(self) -> (*const Self::Element, usize, usize); /// Generate a NULL value. By default this is a NULL items pointer with zero length and /// capacity. @@ -187,36 +249,32 @@ pub(crate) trait PointerArray: Sized { unsafe { Self::from_raw_parts(std::ptr::null(), 0, 0) } } - /// Drop a vector of element pointers. This is a convenience function for implementing + /// Drop a vector of elements. This is a convenience function for implementing /// tc_.._free functions. - fn drop_pointer_vector(mut vec: Vec>) { + fn drop_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()) }); + for e in vec.drain(..) { + // SAFETY: e is a valid Element (caller promisd not to change it) + drop(unsafe { PassByValue::from_arg(e) }); } + // then drop the vector drop(vec); } } impl PassByValue for A where - A: PointerArray, + A: ValueArray, { - type RustType = Vec>; + 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 + // - ValueArray::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. + // - ValueArray::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) } } diff --git a/lib/src/uuidlist.rs b/lib/src/uuidlist.rs new file mode 100644 index 000000000..862ece88e --- /dev/null +++ b/lib/src/uuidlist.rs @@ -0,0 +1,70 @@ +use crate::traits::*; +use crate::uuid::TCUuid; + +/// TCUuidList represents a list of uuids. +/// +/// The content of this struct must be treated as read-only. +#[repr(C)] +pub struct TCUuidList { + /// number of uuids in items + len: libc::size_t, + + /// total size of items (internal use only) + _capacity: libc::size_t, + + /// 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, +} + +impl ValueArray for TCUuidList { + type Element = TCUuid; + + unsafe fn from_raw_parts(items: *const Self::Element, len: usize, cap: usize) -> Self { + TCUuidList { + len, + _capacity: cap, + items, + } + } + + fn into_raw_parts(self) -> (*const Self::Element, usize, usize) { + (self.items, self.len, self._capacity) + } +} + +/// Free a TCUuidList instance. The instance, and all TCUuids it contains, must not be used after +/// this call. +/// +/// When this call returns, the `items` pointer will be NULL, signalling an invalid TCUuidList. +#[no_mangle] +pub unsafe extern "C" fn tc_uuid_list_free(tcuuids: *mut TCUuidList) { + debug_assert!(!tcuuids.is_null()); + // SAFETY: + // - *tcuuids is a valid TCUuidList (caller promises to treat it as read-only) + let uuids = unsafe { TCUuidList::take_from_arg(tcuuids, TCUuidList::null_value()) }; + TCUuidList::drop_vector(uuids); +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn empty_array_has_non_null_pointer() { + let tcuuids = TCUuidList::return_val(Vec::new()); + assert!(!tcuuids.items.is_null()); + assert_eq!(tcuuids.len, 0); + assert_eq!(tcuuids._capacity, 0); + } + + #[test] + fn free_sets_null_pointer() { + let mut tcuuids = TCUuidList::return_val(Vec::new()); + // SAFETY: testing expected behavior + unsafe { tc_uuid_list_free(&mut tcuuids) }; + assert!(tcuuids.items.is_null()); + assert_eq!(tcuuids.len, 0); + assert_eq!(tcuuids._capacity, 0); + } +} diff --git a/lib/taskchampion.h b/lib/taskchampion.h index e59a9c59d..ed8321fd7 100644 --- a/lib/taskchampion.h +++ b/lib/taskchampion.h @@ -148,6 +148,27 @@ typedef struct TCUuid { uint8_t bytes[16]; } TCUuid; +/** + * TCUuidList represents a list of uuids. + * + * The content of this struct must be treated as read-only. + */ +typedef struct TCUuidList { + /** + * number of uuids in items + */ + size_t len; + /** + * total size of items (internal use only) + */ + size_t _capacity; + /** + * 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; +} TCUuidList; + /** * TCStringList represents a list of strings. * @@ -193,6 +214,13 @@ struct TCReplica *tc_replica_new_on_disk(struct TCString *path, struct TCString */ struct TCTaskList tc_replica_all_tasks(struct TCReplica *rep); +/** + * Get a list of all uuids for tasks in the replica. + * + * Returns a TCUuidList with a NULL items field on error. + */ +struct TCUuidList tc_replica_all_task_uuids(struct TCReplica *rep); + /** * Get an existing task by its UUID. * @@ -498,6 +526,14 @@ struct TCString *tc_uuid_to_str(struct TCUuid tcuuid); */ TCResult tc_uuid_from_str(struct TCString *s, struct TCUuid *uuid_out); +/** + * Free a TCUuidList instance. The instance, and all TCUuids it contains, must not be used after + * this call. + * + * When this call returns, the `items` pointer will be NULL, signalling an invalid TCUuidList. + */ +void tc_uuid_list_free(struct TCUuidList *tcuuids); + #ifdef __cplusplus } // extern "C" #endif // __cplusplus