From 175958e75449142f46cccd0289d3e4ca9193fd84 Mon Sep 17 00:00:00 2001 From: Emily Toop Date: Fri, 6 Apr 2018 10:02:42 +0100 Subject: [PATCH] Address review comments @rnewman --- ffi/src/lib.rs | 104 +++++++++++++++++-------------------------- src/conn.rs | 6 +-- src/query_builder.rs | 34 +++++++------- 3 files changed, 60 insertions(+), 84 deletions(-) diff --git a/ffi/src/lib.rs b/ffi/src/lib.rs index 12c3235a..ff52d649 100644 --- a/ffi/src/lib.rs +++ b/ffi/src/lib.rs @@ -57,6 +57,9 @@ pub use utils::strings::{ string_to_c_char, }; +pub type TypedValueIterator = vec::IntoIter; +pub type TypedValueListIterator = vec::IntoIter>; + #[repr(C)] #[derive(Debug, Clone)] pub struct ExternTxReport { @@ -319,80 +322,80 @@ pub unsafe extern "C" fn row_at_index(rows: *mut Vec>, index: c_ } #[no_mangle] -pub unsafe extern "C" fn rows_iter(rows: *mut Vec>) -> *mut vec::IntoIter> { +pub unsafe extern "C" fn rows_iter(rows: *mut Vec>) -> *mut TypedValueListIterator { let result = Box::from_raw(rows); Box::into_raw(Box::new(result.into_iter())) } #[no_mangle] -pub unsafe extern "C" fn rows_iter_next(iter: *mut ::std::vec::IntoIter>) -> *mut Vec { +pub unsafe extern "C" fn rows_iter_next(iter: *mut TypedValueListIterator) -> *mut Vec { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| Box::into_raw(Box::new(v))) } #[no_mangle] -pub unsafe extern "C" fn values_iter(values: *mut Vec) -> *mut vec::IntoIter { +pub unsafe extern "C" fn values_iter(values: *mut Vec) -> *mut TypedValueIterator { let result = Box::from_raw(values); Box::into_raw(Box::new(result.into_iter())) } #[no_mangle] -pub unsafe extern "C" fn values_iter_next(iter: *mut vec::IntoIter) -> *const TypedValue { +pub unsafe extern "C" fn values_iter_next(iter: *mut TypedValueIterator) -> *const TypedValue { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| &v as *const TypedValue) } //as_long #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_long(iter: *mut vec::IntoIter) -> *const i64 { +pub unsafe extern "C" fn values_iter_next_as_long(iter: *mut TypedValueIterator) -> *const i64 { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| &v.into_long().expect("Typed value cannot be coerced into a Long") as *const i64) } // as ref #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_entid(iter: *mut vec::IntoIter) -> *const Entid { +pub unsafe extern "C" fn values_iter_next_as_entid(iter: *mut TypedValueIterator) -> *const Entid { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| &v.into_entid().expect("Typed value cannot be coerced into am Entid") as *const Entid) } // as kw #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_kw(iter: *mut vec::IntoIter) -> *const c_char { +pub unsafe extern "C" fn values_iter_next_as_kw(iter: *mut TypedValueIterator) -> *const c_char { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| string_to_c_char(v.into_kw().expect("Typed value cannot be coerced into a Namespaced Keyword").to_string())) } //as_boolean #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_boolean(iter: *mut vec::IntoIter) -> *const bool { +pub unsafe extern "C" fn values_iter_next_as_boolean(iter: *mut TypedValueIterator) -> *const bool { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| &v.into_boolean().expect("Typed value cannot be coerced into a Boolean") as *const bool) } //as_double #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_double(iter: *mut vec::IntoIter) -> *const f64 { +pub unsafe extern "C" fn values_iter_next_as_double(iter: *mut TypedValueIterator) -> *const f64 { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| &v.into_double().expect("Typed value cannot be coerced into a Double") as *const f64) } //as_timestamp #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_timestamp(iter: *mut vec::IntoIter) -> *const i64 { +pub unsafe extern "C" fn values_iter_next_as_timestamp(iter: *mut TypedValueIterator) -> *const i64 { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| v.into_timestamp().expect("Typed value cannot be coerced into a Timestamp") as *const i64) } //as_string #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_string(iter: *mut vec::IntoIter) -> *const c_char { +pub unsafe extern "C" fn values_iter_next_as_string(iter: *mut TypedValueIterator) -> *const c_char { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| c_char_from_rc(v.into_string().expect("Typed value cannot be coerced into a String"))) } //as_uuid #[no_mangle] -pub unsafe extern "C" fn values_iter_next_as_uuid(iter: *mut vec::IntoIter) -> *const c_char { +pub unsafe extern "C" fn values_iter_next_as_uuid(iter: *mut TypedValueIterator) -> *const c_char { let iter = &mut *iter; iter.next().map_or(std::ptr::null_mut(), |v| string_to_c_char(v.into_uuid_string().expect("Typed value cannot be coerced into a Uuid"))) } @@ -556,11 +559,11 @@ pub unsafe extern "C" fn store_sync(store: *mut Store, user_uuid: *const c_char, Box::into_raw(Box::new(res.into())) } -fn add_value_for_attribute(store: &mut Store, entid: E, attribute: String, value: V) -> *mut ExternResult +fn assert_datom(store: &mut Store, entid: E, attribute: String, value: V) -> *mut ExternResult where E: Into, V: Into { let kw = kw_from_string(attribute); - let res = store.add_value_for_attribute(entid.into(), kw, value.into()); + let res = store.assert_datom(entid.into(), kw, value.into()); Box::into_raw(Box::new(res.into())) } @@ -568,7 +571,7 @@ where E: Into, pub unsafe extern "C" fn store_set_long_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: i64) -> *mut ExternResult { let store = &mut*store; let kw = kw_from_string(c_char_to_string(attribute)); - let res = store.add_value_for_attribute(KnownEntid(entid), kw, TypedValue::Long(value)); + let res = store.assert_datom(KnownEntid(entid), kw, TypedValue::Long(value)); Box::into_raw(Box::new(res.into())) } @@ -576,7 +579,7 @@ pub unsafe extern "C" fn store_set_long_for_attribute_on_entid(store: *mut Store pub unsafe extern "C" fn store_set_entid_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: Entid) -> *mut ExternResult { let store = &mut*store; let kw = kw_from_string(c_char_to_string(attribute)); - let res = store.add_value_for_attribute(KnownEntid(entid), kw, TypedValue::Ref(value)); + let res = store.assert_datom(KnownEntid(entid), kw, TypedValue::Ref(value)); Box::into_raw(Box::new(res.into())) } @@ -590,41 +593,41 @@ pub unsafe extern "C" fn store_set_kw_ref_for_attribute_on_entid(store: *mut Sto return Box::into_raw(Box::new(ExternResult { ok: std::ptr::null_mut(), err: string_to_c_char(format!("Unknown attribute {:?}", value)) })); } let kw_entid = is_valid.unwrap(); - let res = store.add_value_for_attribute(KnownEntid(entid), kw, TypedValue::Ref(kw_entid.into())); + let res = store.assert_datom(KnownEntid(entid), kw, TypedValue::Ref(kw_entid.into())); Box::into_raw(Box::new(res.into())) } #[no_mangle] pub unsafe extern "C" fn store_set_boolean_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: bool) -> *mut ExternResult { let store = &mut*store; - add_value_for_attribute(store, KnownEntid(entid), c_char_to_string(attribute), value) + assert_datom(store, KnownEntid(entid), c_char_to_string(attribute), value) } #[no_mangle] pub unsafe extern "C" fn store_set_double_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: f64) -> *mut ExternResult { let store = &mut*store; - add_value_for_attribute(store, KnownEntid(entid), c_char_to_string(attribute), value) + assert_datom(store, KnownEntid(entid), c_char_to_string(attribute), value) } #[no_mangle] pub unsafe extern "C" fn store_set_timestamp_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: time_t) -> *mut ExternResult { let store = &mut*store; let kw = kw_from_string(c_char_to_string(attribute)); - let res = store.add_value_for_attribute(KnownEntid(entid), kw, TypedValue::instant(value as i64)); + let res = store.assert_datom(KnownEntid(entid), kw, TypedValue::instant(value as i64)); Box::into_raw(Box::new(res.into())) } #[no_mangle] pub unsafe extern "C" fn store_set_string_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: *const c_char) -> *mut ExternResult { let store = &mut*store; - add_value_for_attribute(store, KnownEntid(entid), c_char_to_string(attribute), c_char_to_string(value)) + assert_datom(store, KnownEntid(entid), c_char_to_string(attribute), c_char_to_string(value)) } #[no_mangle] pub unsafe extern "C" fn store_set_uuid_for_attribute_on_entid(store: *mut Store, entid: Entid, attribute: *const c_char, value: *const c_char) -> *mut ExternResult { let store = &mut*store; let uuid = Uuid::parse_str(&c_char_to_string(value)).expect("valid uuid"); - add_value_for_attribute(store, KnownEntid(entid), c_char_to_string(attribute), uuid) + assert_datom(store, KnownEntid(entid), c_char_to_string(attribute), uuid) } #[no_mangle] @@ -635,51 +638,24 @@ pub unsafe extern "C" fn destroy(obj: *mut c_void) { } } -#[no_mangle] -pub unsafe extern "C" fn query_builder_destroy(obj: *mut QueryBuilder) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +macro_rules! define_destructor ( + ($name:ident, $t:ty) => ( + #[no_mangle] + pub unsafe extern "C" fn $name(obj: *mut $t) { + if !obj.is_null() { let _ = Box::from_raw(obj); } + } + ) +); +define_destructor!(query_builder_destroy, QueryBuilder); -#[no_mangle] -pub unsafe extern "C" fn store_destroy(obj: *mut Store) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(store_destroy, Store); -#[no_mangle] -pub unsafe extern "C" fn typed_value_destroy(obj: *mut TypedValue) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(typed_value_destroy, TypedValue); -#[no_mangle] -pub unsafe extern "C" fn typed_value_list_destroy(obj: *mut Vec) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(typed_value_list_destroy, Vec); -#[no_mangle] -pub unsafe extern "C" fn typed_value_list_iter_destroy(obj: *mut vec::IntoIter) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(typed_value_list_iter_destroy, TypedValueIterator); -#[no_mangle] -pub unsafe extern "C" fn typed_value_result_set_destroy(obj: *mut Vec>) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(typed_value_result_set_destroy, Vec>); -#[no_mangle] -pub unsafe extern "C" fn typed_value_result_set_iter_destroy(obj: *mut vec::IntoIter>) { - if !obj.is_null() { - let _ = Box::from_raw(obj); - } -} +define_destructor!(typed_value_result_set_iter_destroy, TypedValueListIterator); diff --git a/src/conn.rs b/src/conn.rs index d2439fea..14626791 100644 --- a/src/conn.rs +++ b/src/conn.rs @@ -588,8 +588,8 @@ impl Store { self.conn.unregister_observer(key); } - pub fn add_value_for_attribute(&mut self, entid: T, attribute: NamespacedKeyword, value: TypedValue) -> Result<()> where T: Into { - self.conn.add_value_for_attribute(&mut self.sqlite, entid, attribute, value) + pub fn assert_datom(&mut self, entid: T, attribute: NamespacedKeyword, value: TypedValue) -> Result<()> where T: Into { + self.conn.assert_datom(&mut self.sqlite, entid, attribute, value) } } @@ -882,7 +882,7 @@ impl Conn { // TODO: expose the entity builder over FFI and remove the need for this function entirely // It's really only here in order to keep the FFI layer as thin as possible. // Once the entity builder is exposed, we can perform all of these functions over FFI from the client. - pub fn add_value_for_attribute(&mut self, sqlite: &mut rusqlite::Connection, entid: T, attribute: NamespacedKeyword, value: TypedValue) -> Result<()> where T: Into { + pub fn assert_datom(&mut self, sqlite: &mut rusqlite::Connection, entid: T, attribute: NamespacedKeyword, value: TypedValue) -> Result<()> where T: Into { let in_progress = self.begin_transaction(sqlite)?; let mut builder = in_progress.builder().describe(entid.into()); builder.add_kw(&attribute, value)?; diff --git a/src/query_builder.rs b/src/query_builder.rs index 4e4a2da0..54a0ff2b 100644 --- a/src/query_builder.rs +++ b/src/query_builder.rs @@ -131,8 +131,8 @@ mod test { let yes = report.tempids.get("u").expect("found it").clone(); let entid = QueryBuilder::new(&mut store, r#"[:find ?x . - :in ?v - :where [?x :foo/boolean ?v]]"#) + :in ?v + :where [?x :foo/boolean ?v]]"#) .bind_value("?v", true) .execute_scalar().expect("ScalarResult") .map_or(None, |t| t.into_entid()); @@ -169,8 +169,8 @@ mod test { let n_yes = report.tempids.get("n").expect("found it").clone(); let entids: Vec = QueryBuilder::new(&mut store, r#"[:find [?x ...] - :in ?v - :where [?x :foo/boolean ?v]]"#) + :in ?v + :where [?x :foo/boolean ?v]]"#) .bind_value("?v", true) .execute_coll().expect("CollResult") .into_iter() @@ -208,8 +208,8 @@ mod test { let n_yes = report.tempids.get("n").expect("found it").clone(); let results = QueryBuilder::new(&mut store, r#"[:find [?x ...] - :in ?v - :where [?x :foo/boolean ?v]]"#) + :in ?v + :where [?x :foo/boolean ?v]]"#) .bind_value("?v", true) .execute_coll().expect("CollResult"); let entid = results.get(1).map_or(None, |t| t.to_owned().into_entid()).expect("entid"); @@ -245,9 +245,9 @@ mod test { let n_yes = report.tempids.get("n").expect("found it").clone(); let results = QueryBuilder::new(&mut store, r#"[:find [?x, ?i] - :in ?v ?i - :where [?x :foo/boolean ?v] - [?x :foo/long ?i]]"#) + :in ?v ?i + :where [?x :foo/boolean ?v] + [?x :foo/long ?i]]"#) .bind_value("?v", true) .bind_long("?i", 27) .execute_tuple().expect("TupleResult").expect("Vec"); @@ -286,9 +286,9 @@ mod test { let n_yes = report.tempids.get("n").expect("found it").clone(); let results: Vec = QueryBuilder::new(&mut store, r#"[:find [?x, ?i] - :in ?v ?i - :where [?x :foo/boolean ?v] - [?x :foo/long ?i]]"#) + :in ?v ?i + :where [?x :foo/boolean ?v] + [?x :foo/long ?i]]"#) .bind_value("?v", true) .bind_long("?i", 27) .execute_tuple().expect("TupleResult").unwrap_or(vec![]); @@ -331,8 +331,8 @@ mod test { }; let mut results: Vec = QueryBuilder::new(&mut store, r#"[:find ?x ?v ?i - :where [?x :foo/boolean ?v] - [?x :foo/long ?i]]"#) + :where [?x :foo/boolean ?v] + [?x :foo/long ?i]]"#) .execute_rel().expect("RelResult") .into_iter() .map(|row| { @@ -377,9 +377,9 @@ mod test { let l_yes = report.tempids.get("l").expect("found it").clone(); let results = QueryBuilder::new(&mut store, r#"[:find [?v ?i] - :in ?x - :where [?x :foo/boolean ?v] - [?x :foo/long ?i]]"#) + :in ?x + :where [?x :foo/boolean ?v] + [?x :foo/long ?i]]"#) .bind_ref("?x", l_yes) .execute_tuple().expect("TupleResult") .unwrap_or(vec![]);