Allow taking from pointer lists

This introduces `tc_task_list_take`, supporting taking ownership of an
item in a task list.

TCTaskList is the only pointer list, but this is a generic and could be
used for other types.
This commit is contained in:
Dustin J. Mitchell 2022-03-13 00:08:55 +00:00
parent 9355e1a728
commit 33a3b980d0
No known key found for this signature in database
10 changed files with 278 additions and 52 deletions

View file

@ -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();
}

View file

@ -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)
}
}

View file

@ -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)
}
}

View file

@ -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:

View file

@ -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)
}
}

View file

@ -171,7 +171,10 @@ impl TryFrom<RustString<'static>> 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<TCTask>,
items: *mut Option<NonNull<TCTask>>,
}
impl CList for TCTaskList {
type Element = NonNull<TCTask>;
type Element = Option<NonNull<TCTask>>;
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);
}
}

View file

@ -150,13 +150,17 @@ pub(crate) trait PassByPointer: Sized {
/// The PassByValue trait will be implemented automatically, converting between the C type and
/// `Vec<Element>`.
///
/// 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<T>` or `Option<NonNull<T>>` 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<CL, T>(list: *mut CL)
where
CL: CList<Element = NonNull<T>>,
@ -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<CL, T>(list: *mut CL)
where
CL: CList<Element = Option<NonNull<T>>>,
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<CL, T>(
list: *mut CL,
index: usize,
) -> Option<NonNull<T>>
where
CL: CList<Element = Option<NonNull<T>>>,
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<A> PassByValue for A
where
A: CList,

View file

@ -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)
}
}

View file

@ -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)
}
}

View file

@ -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