From 45d3e38c63dbc0fa3844bfceacaa41bdef647ebc Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sat, 9 Jan 2021 22:09:06 +0000 Subject: [PATCH] Always pass Uuids by value Rust handles this well. Fixes #125. --- cli/src/invocation/filter.rs | 20 ++++++++-------- cli/src/invocation/report.rs | 20 ++++++++-------- taskchampion/src/replica.rs | 30 ++++++++++++------------ taskchampion/src/task.rs | 8 +++---- taskchampion/src/taskdb.rs | 18 +++++++------- taskchampion/src/taskstorage/inmemory.rs | 24 +++++++++---------- taskchampion/src/taskstorage/kv.rs | 30 ++++++++++++------------ taskchampion/src/taskstorage/mod.rs | 6 ++--- 8 files changed, 78 insertions(+), 78 deletions(-) diff --git a/cli/src/invocation/filter.rs b/cli/src/invocation/filter.rs index 0cab337ae..f9440dcd9 100644 --- a/cli/src/invocation/filter.rs +++ b/cli/src/invocation/filter.rs @@ -125,18 +125,18 @@ pub(super) fn filtered_tasks( let task = match id { TaskId::WorkingSetId(id) => replica.get_working_set_task(*id)?, TaskId::PartialUuid(_) => unreachable!(), // not present in absolute id list - TaskId::Uuid(id) => replica.get_task(id)?, + TaskId::Uuid(id) => replica.get_task(*id)?, }; if let Some(task) = task { // if we have already seen this task, skip ahead.. - let uuid = *task.get_uuid(); + let uuid = task.get_uuid(); if seen.contains(&uuid) { continue; } seen.insert(uuid); - let working_set_id = replica.get_working_set_index(&uuid)?; + let working_set_id = replica.get_working_set_index(uuid)?; if match_task(filter, &task, uuid, working_set_id) { res.push(task); @@ -150,7 +150,7 @@ pub(super) fn filtered_tasks( log::debug!("Scanning all tasks in the task database"); for (uuid, task) in replica.all_tasks()?.drain() { // Yikes, slow! https://github.com/djmitche/taskchampion/issues/108 - let working_set_id = replica.get_working_set_index(&uuid)?; + let working_set_id = replica.get_working_set_index(uuid)?; if match_task(filter, &task, uuid, working_set_id) { res.push(task); } @@ -160,7 +160,7 @@ pub(super) fn filtered_tasks( log::debug!("Scanning only the working set (pending tasks)"); for (i, task) in replica.working_set()?.drain(..).enumerate() { if let Some(task) = task { - let uuid = *task.get_uuid(); + let uuid = task.get_uuid(); if match_task(filter, &task, uuid, Some(i)) { res.push(task); } @@ -186,13 +186,13 @@ mod test { let _t = replica.new_task(Status::Pending, s!("C")).unwrap(); replica.rebuild_working_set(true).unwrap(); - let t1uuid = *t1.get_uuid(); + let t1uuid = t1.get_uuid(); let filter = Filter { conditions: vec![Condition::IdList(vec![ - TaskId::Uuid(t1uuid), // A - TaskId::WorkingSetId(1), // A (again, dups filtered) - TaskId::Uuid(*t2.get_uuid()), // B + TaskId::Uuid(t1uuid), // A + TaskId::WorkingSetId(1), // A (again, dups filtered) + TaskId::Uuid(t2.get_uuid()), // B ])], }; let mut filtered: Vec<_> = filtered_tasks(&mut replica, &filter) @@ -212,7 +212,7 @@ mod test { let _t = replica.new_task(Status::Pending, s!("C")).unwrap(); replica.rebuild_working_set(true).unwrap(); - let t1uuid = *t1.get_uuid(); + let t1uuid = t1.get_uuid(); let t2uuid = t2.get_uuid().to_string(); let t2partial = t2uuid[..13].to_owned(); diff --git a/cli/src/invocation/report.rs b/cli/src/invocation/report.rs index c1e744238..12497553c 100644 --- a/cli/src/invocation/report.rs +++ b/cli/src/invocation/report.rs @@ -18,15 +18,15 @@ impl WorkingSet { Ok(Self( working_set .iter() - .map(|opt| opt.as_ref().map(|t| *t.get_uuid())) + .map(|opt| opt.as_ref().map(|t| t.get_uuid())) .collect(), )) } - fn index(&self, target: &Uuid) -> Option { + fn index(&self, target: Uuid) -> Option { for (i, uuid) in self.0.iter().enumerate() { if let Some(uuid) = uuid { - if uuid == target { + if *uuid == target { return Some(i); } } @@ -51,10 +51,10 @@ fn sort_tasks(tasks: &mut Vec, report: &Report, working_set: &WorkingSet) (Some(a_id), Some(b_id)) => a_id.cmp(&b_id), (Some(_), None) => Ordering::Less, (None, Some(_)) => Ordering::Greater, - (None, None) => a_uuid.cmp(b_uuid), + (None, None) => a_uuid.cmp(&b_uuid), } } - SortBy::Uuid => a.get_uuid().cmp(b.get_uuid()), + SortBy::Uuid => a.get_uuid().cmp(&b.get_uuid()), SortBy::Description => a.get_description().cmp(b.get_description()), }; // If this sort property is equal, go on to the next.. @@ -165,7 +165,7 @@ mod test { replica.rebuild_working_set(true).unwrap(); - [*t1.get_uuid(), *t2.get_uuid(), *t3.get_uuid()] + [t1.get_uuid(), t2.get_uuid(), t3.get_uuid()] } #[test] @@ -237,7 +237,7 @@ mod test { let mut tasks: Vec<_> = replica.all_tasks().unwrap().values().cloned().collect(); sort_tasks(&mut tasks, &report, &working_set); - let got_uuids: Vec<_> = tasks.iter().map(|t| *t.get_uuid()).collect(); + let got_uuids: Vec<_> = tasks.iter().map(|t| t.get_uuid()).collect(); let mut exp_uuids = uuids.to_vec(); exp_uuids.sort(); assert_eq!(got_uuids, exp_uuids); @@ -291,7 +291,7 @@ mod test { // get the task that's not in the working set, which should show // a uuid for its id column - let task = replica.get_task(&uuids[1]).unwrap().unwrap(); + let task = replica.get_task(uuids[1]).unwrap().unwrap(); assert_eq!( task_column(&task, &column, &working_set), uuids[1].to_string() @@ -323,7 +323,7 @@ mod test { // make task A active replica - .get_task(&uuids[0]) + .get_task(uuids[0]) .unwrap() .unwrap() .into_mut(&mut replica) @@ -363,7 +363,7 @@ mod test { // add some tags to task A let mut t1 = replica - .get_task(&uuids[0]) + .get_task(uuids[0]) .unwrap() .unwrap() .into_mut(&mut replica); diff --git a/taskchampion/src/replica.rs b/taskchampion/src/replica.rs index 7c3df1e30..fa20c7c3b 100644 --- a/taskchampion/src/replica.rs +++ b/taskchampion/src/replica.rs @@ -69,7 +69,7 @@ impl Replica { } /// Add the given uuid to the working set, returning its index. - pub(crate) fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible { + pub(crate) fn add_to_working_set(&mut self, uuid: Uuid) -> Fallible { self.taskdb.add_to_working_set(uuid) } @@ -94,7 +94,7 @@ impl Replica { let mut res = Vec::with_capacity(working_set.len()); for item in working_set.iter() { res.push(match item { - Some(u) => match self.taskdb.get_task(u)? { + Some(u) => match self.taskdb.get_task(*u)? { Some(tm) => Some(Task::new(*u, tm)), None => None, }, @@ -105,11 +105,11 @@ impl Replica { } /// Get an existing task by its UUID - pub fn get_task(&mut self, uuid: &Uuid) -> Fallible> { + pub fn get_task(&mut self, uuid: Uuid) -> Fallible> { Ok(self .taskdb .get_task(uuid)? - .map(move |tm| Task::new(*uuid, tm))) + .map(move |tm| Task::new(uuid, tm))) } /// Get an existing task by its working set index @@ -119,7 +119,7 @@ impl Replica { if let Some(uuid) = working_set[i as usize] { return Ok(self .taskdb - .get_task(&uuid)? + .get_task(uuid)? .map(move |tm| Task::new(uuid, tm))); } } @@ -127,11 +127,11 @@ impl Replica { } /// Get the working set index for the given task uuid - pub fn get_working_set_index(&mut self, uuid: &Uuid) -> Fallible> { + pub fn get_working_set_index(&mut self, uuid: Uuid) -> Fallible> { let working_set = self.taskdb.working_set()?; for (i, u) in working_set.iter().enumerate() { - if let Some(ref u) = u { - if u == uuid { + if let Some(u) = u { + if *u == uuid { return Ok(Some(i)); } } @@ -154,13 +154,13 @@ impl Replica { /// Deleted; this is the final purge of the task. This is not a public method as deletion /// should only occur through expiration. #[allow(dead_code)] - fn delete_task(&mut self, uuid: &Uuid) -> Fallible<()> { + fn delete_task(&mut self, uuid: Uuid) -> Fallible<()> { // check that it already exists; this is a convenience check, as the task may already exist // when this Create operation is finally sync'd with operations from other replicas if self.taskdb.get_task(uuid)?.is_none() { return Err(Error::DBError(format!("Task {} does not exist", uuid)).into()); } - self.taskdb.apply(Operation::Delete { uuid: *uuid })?; + self.taskdb.apply(Operation::Delete { uuid })?; trace!("task {} deleted", uuid); Ok(()) } @@ -220,7 +220,7 @@ mod tests { assert_eq!(t.get_status(), Status::Completed); // check tha values have changed in storage, too - let t = rep.get_task(&t.get_uuid()).unwrap().unwrap(); + let t = rep.get_task(t.get_uuid()).unwrap().unwrap(); assert_eq!(t.get_description(), "past tense"); assert_eq!(t.get_status(), Status::Completed); } @@ -233,7 +233,7 @@ mod tests { let uuid = t.get_uuid(); rep.delete_task(uuid).unwrap(); - assert_eq!(rep.get_task(&uuid).unwrap(), None); + assert_eq!(rep.get_task(uuid).unwrap(), None); } #[test] @@ -245,14 +245,14 @@ mod tests { .unwrap(); let uuid = t.get_uuid(); - let t = rep.get_task(&uuid).unwrap().unwrap(); + let t = rep.get_task(uuid).unwrap().unwrap(); assert_eq!(t.get_description(), String::from("another task")); let mut t = t.into_mut(&mut rep); t.set_status(Status::Deleted).unwrap(); t.set_description("gone".into()).unwrap(); - let t = rep.get_task(&uuid).unwrap().unwrap(); + let t = rep.get_task(uuid).unwrap().unwrap(); assert_eq!(t.get_status(), Status::Deleted); assert_eq!(t.get_description(), "gone"); @@ -286,6 +286,6 @@ mod tests { fn get_does_not_exist() { let mut rep = Replica::new_inmemory(); let uuid = Uuid::new_v4(); - assert_eq!(rep.get_task(&uuid).unwrap(), None); + assert_eq!(rep.get_task(uuid).unwrap(), None); } } diff --git a/taskchampion/src/task.rs b/taskchampion/src/task.rs index 8736a97da..99b92873a 100644 --- a/taskchampion/src/task.rs +++ b/taskchampion/src/task.rs @@ -167,8 +167,8 @@ impl Task { Task { uuid, taskmap } } - pub fn get_uuid(&self) -> &Uuid { - &self.uuid + pub fn get_uuid(&self) -> Uuid { + self.uuid } pub fn get_taskmap(&self) -> &TaskMap { @@ -254,7 +254,7 @@ impl<'r> TaskMut<'r> { pub fn set_status(&mut self, status: Status) -> Fallible<()> { if status == Status::Pending { let uuid = self.uuid; - self.replica.add_to_working_set(&uuid)?; + self.replica.add_to_working_set(uuid)?; } self.set_string("status", Some(String::from(status.to_taskmap()))) } @@ -353,7 +353,7 @@ impl<'r> TaskMut<'r> { #[cfg(test)] fn reload(&mut self) -> Fallible<()> { let uuid = self.uuid; - let task = self.replica.get_task(&uuid)?.unwrap(); + let task = self.replica.get_task(uuid)?.unwrap(); self.task.taskmap = task.taskmap; Ok(()) } diff --git a/taskchampion/src/taskdb.rs b/taskchampion/src/taskdb.rs index 71e49b042..c04a26c36 100644 --- a/taskchampion/src/taskdb.rs +++ b/taskchampion/src/taskdb.rs @@ -54,7 +54,7 @@ impl TaskDB { } } Operation::Delete { ref uuid } => { - if !txn.delete_task(uuid)? { + if !txn.delete_task(*uuid)? { return Err(Error::DBError(format!("Task {} does not exist", uuid)).into()); } } @@ -65,7 +65,7 @@ impl TaskDB { timestamp: _, } => { // update if this task exists, otherwise ignore - if let Some(mut task) = txn.get_task(uuid)? { + if let Some(mut task) = txn.get_task(*uuid)? { match value { Some(ref val) => task.insert(property.to_string(), val.clone()), None => task.remove(property), @@ -99,7 +99,7 @@ impl TaskDB { } /// Get a single task, by uuid. - pub fn get_task(&mut self, uuid: &Uuid) -> Fallible> { + pub fn get_task(&mut self, uuid: Uuid) -> Fallible> { let mut txn = self.storage.txn()?; txn.get_task(uuid) } @@ -123,7 +123,7 @@ impl TaskDB { // working set. for elt in txn.get_working_set()? { if let Some(uuid) = elt { - if let Some(task) = txn.get_task(&uuid)? { + if let Some(task) = txn.get_task(uuid)? { if in_working_set(&task) { new_ws.push(Some(uuid)); seen.insert(uuid); @@ -143,7 +143,7 @@ impl TaskDB { txn.clear_working_set()?; for elt in new_ws.drain(0..new_ws.len()) { if let Some(uuid) = elt { - txn.add_to_working_set(&uuid)?; + txn.add_to_working_set(uuid)?; } } } else { @@ -159,7 +159,7 @@ impl TaskDB { // end of the list, whether renumbering or not for (uuid, task) in txn.all_tasks()? { if !seen.contains(&uuid) && in_working_set(&task) { - txn.add_to_working_set(&uuid)?; + txn.add_to_working_set(uuid)?; } } @@ -169,11 +169,11 @@ impl TaskDB { /// Add the given uuid to the working set and return its index; if it is already in the working /// set, its index is returned. This does *not* renumber any existing tasks. - pub fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible { + pub fn add_to_working_set(&mut self, uuid: Uuid) -> Fallible { let mut txn = self.storage.txn()?; // search for an existing entry for this task.. for (i, elt) in txn.get_working_set()?.iter().enumerate() { - if *elt == Some(*uuid) { + if *elt == Some(uuid) { // (note that this drops the transaction with no changes made) return Ok(i); } @@ -544,7 +544,7 @@ mod tests { txn.clear_working_set()?; for i in &[1usize, 3, 4] { - txn.add_to_working_set(&uuids[*i])?; + txn.add_to_working_set(uuids[*i])?; } txn.commit()?; diff --git a/taskchampion/src/taskstorage/inmemory.rs b/taskchampion/src/taskstorage/inmemory.rs index 163312f8c..718ff1b1e 100644 --- a/taskchampion/src/taskstorage/inmemory.rs +++ b/taskchampion/src/taskstorage/inmemory.rs @@ -43,8 +43,8 @@ impl<'t> Txn<'t> { } impl<'t> TaskStorageTxn for Txn<'t> { - fn get_task(&mut self, uuid: &Uuid) -> Fallible> { - match self.data_ref().tasks.get(uuid) { + fn get_task(&mut self, uuid: Uuid) -> Fallible> { + match self.data_ref().tasks.get(&uuid) { None => Ok(None), Some(t) => Ok(Some(t.clone())), } @@ -64,8 +64,8 @@ impl<'t> TaskStorageTxn for Txn<'t> { Ok(()) } - fn delete_task(&mut self, uuid: &Uuid) -> Fallible { - Ok(self.mut_data_ref().tasks.remove(uuid).is_some()) + fn delete_task(&mut self, uuid: Uuid) -> Fallible { + Ok(self.mut_data_ref().tasks.remove(&uuid).is_some()) } fn all_tasks<'a>(&mut self) -> Fallible> { @@ -108,9 +108,9 @@ impl<'t> TaskStorageTxn for Txn<'t> { Ok(self.data_ref().working_set.clone()) } - fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible { + fn add_to_working_set(&mut self, uuid: Uuid) -> Fallible { let working_set = &mut self.mut_data_ref().working_set; - working_set.push(Some(*uuid)); + working_set.push(Some(uuid)); Ok(working_set.len()) } @@ -194,8 +194,8 @@ mod test { { let mut txn = storage.txn()?; - txn.add_to_working_set(&uuid1)?; - txn.add_to_working_set(&uuid2)?; + txn.add_to_working_set(uuid1)?; + txn.add_to_working_set(uuid2)?; txn.commit()?; } @@ -216,16 +216,16 @@ mod test { { let mut txn = storage.txn()?; - txn.add_to_working_set(&uuid1)?; - txn.add_to_working_set(&uuid2)?; + txn.add_to_working_set(uuid1)?; + txn.add_to_working_set(uuid2)?; txn.commit()?; } { let mut txn = storage.txn()?; txn.clear_working_set()?; - txn.add_to_working_set(&uuid2)?; - txn.add_to_working_set(&uuid1)?; + txn.add_to_working_set(uuid2)?; + txn.add_to_working_set(uuid1)?; txn.commit()?; } diff --git a/taskchampion/src/taskstorage/kv.rs b/taskchampion/src/taskstorage/kv.rs index 65d1f331f..4bbd087be 100644 --- a/taskchampion/src/taskstorage/kv.rs +++ b/taskchampion/src/taskstorage/kv.rs @@ -105,7 +105,7 @@ impl<'t> Txn<'t> { } impl<'t> TaskStorageTxn for Txn<'t> { - fn get_task(&mut self, uuid: &Uuid) -> Fallible> { + fn get_task(&mut self, uuid: Uuid) -> Fallible> { let bucket = self.tasks_bucket(); let buf = match self.kvtxn().get(bucket, uuid.into()) { Ok(buf) => buf, @@ -136,7 +136,7 @@ impl<'t> TaskStorageTxn for Txn<'t> { Ok(()) } - fn delete_task(&mut self, uuid: &Uuid) -> Fallible { + fn delete_task(&mut self, uuid: Uuid) -> Fallible { let bucket = self.tasks_bucket(); let kvtxn = self.kvtxn(); match kvtxn.del(bucket, uuid.into()) { @@ -275,7 +275,7 @@ impl<'t> TaskStorageTxn for Txn<'t> { Ok(res) } - fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible { + fn add_to_working_set(&mut self, uuid: Uuid) -> Fallible { let working_set_bucket = self.working_set_bucket(); let numbers_bucket = self.numbers_bucket(); let kvtxn = self.kvtxn(); @@ -289,7 +289,7 @@ impl<'t> TaskStorageTxn for Txn<'t> { kvtxn.set( working_set_bucket, next_index.into(), - Msgpack::to_value_buf(*uuid)?, + Msgpack::to_value_buf(uuid)?, )?; kvtxn.set( numbers_bucket, @@ -372,7 +372,7 @@ mod test { } { let mut txn = storage.txn()?; - let task = txn.get_task(&uuid)?; + let task = txn.get_task(uuid)?; assert_eq!(task, Some(taskmap_with(vec![]))); } Ok(()) @@ -403,7 +403,7 @@ mod test { let uuid = Uuid::new_v4(); { let mut txn = storage.txn()?; - let task = txn.get_task(&uuid)?; + let task = txn.get_task(uuid)?; assert_eq!(task, None); } Ok(()) @@ -421,7 +421,7 @@ mod test { } { let mut txn = storage.txn()?; - let task = txn.get_task(&uuid)?; + let task = txn.get_task(uuid)?; assert_eq!( task, Some(taskmap_with(vec![("k".to_string(), "v".to_string())])) @@ -437,7 +437,7 @@ mod test { let uuid = Uuid::new_v4(); { let mut txn = storage.txn()?; - assert!(!txn.delete_task(&uuid)?); + assert!(!txn.delete_task(uuid)?); } Ok(()) } @@ -454,7 +454,7 @@ mod test { } { let mut txn = storage.txn()?; - assert!(txn.delete_task(&uuid)?); + assert!(txn.delete_task(uuid)?); } Ok(()) } @@ -640,8 +640,8 @@ mod test { { let mut txn = storage.txn()?; - txn.add_to_working_set(&uuid1)?; - txn.add_to_working_set(&uuid2)?; + txn.add_to_working_set(uuid1)?; + txn.add_to_working_set(uuid2)?; txn.commit()?; } @@ -663,16 +663,16 @@ mod test { { let mut txn = storage.txn()?; - txn.add_to_working_set(&uuid1)?; - txn.add_to_working_set(&uuid2)?; + txn.add_to_working_set(uuid1)?; + txn.add_to_working_set(uuid2)?; txn.commit()?; } { let mut txn = storage.txn()?; txn.clear_working_set()?; - txn.add_to_working_set(&uuid2)?; - txn.add_to_working_set(&uuid1)?; + txn.add_to_working_set(uuid2)?; + txn.add_to_working_set(uuid1)?; txn.commit()?; } diff --git a/taskchampion/src/taskstorage/mod.rs b/taskchampion/src/taskstorage/mod.rs index 2b1eecbb5..571c0d8b5 100644 --- a/taskchampion/src/taskstorage/mod.rs +++ b/taskchampion/src/taskstorage/mod.rs @@ -44,7 +44,7 @@ pub(crate) const DEFAULT_BASE_VERSION: Uuid = crate::server::NO_VERSION_ID; /// It is safe and performant to drop transactions that did not modify any data without committing. pub trait TaskStorageTxn { /// Get an (immutable) task, if it is in the storage - fn get_task(&mut self, uuid: &Uuid) -> Fallible>; + fn get_task(&mut self, uuid: Uuid) -> Fallible>; /// Create an (empty) task, only if it does not already exist. Returns true if /// the task was created (did not already exist). @@ -55,7 +55,7 @@ pub trait TaskStorageTxn { fn set_task(&mut self, uuid: Uuid, task: TaskMap) -> Fallible<()>; /// Delete a task, if it exists. Returns true if the task was deleted (already existed) - fn delete_task(&mut self, uuid: &Uuid) -> Fallible; + fn delete_task(&mut self, uuid: Uuid) -> Fallible; /// Get the uuids and bodies of all tasks in the storage, in undefined order. fn all_tasks(&mut self) -> Fallible>; @@ -86,7 +86,7 @@ pub trait TaskStorageTxn { /// Add a task to the working set and return its (one-based) index. This index will be one greater /// than the highest used index. - fn add_to_working_set(&mut self, uuid: &Uuid) -> Fallible; + fn add_to_working_set(&mut self, uuid: Uuid) -> Fallible; /// Update the working set task at the given index. This cannot add a new item to the /// working set.