Move crsql_createCrr to Rust #349
Conversation
| if self.len() == 0 { | ||
| null_mut() | ||
| } else { | ||
| self.shrink_to(0); |
There was a problem hiding this comment.
why shrink before converting to raw parts?
There was a problem hiding this comment.
Because Vec also has a capacity which might be larger than its length. When we transfer ownership out of Rust we need to strip this unused space to avoid memory leaks. Essentially we make sure that capacity = length which is what we assume when transferring ownership back to Rust to free all the stuff.
| */ | ||
| pub fn create_crr( | ||
| db: *mut sqlite::sqlite3, | ||
| _schema: &str, |
There was a problem hiding this comment.
one day we'll support attached dbs in cr-sqlite...
|
also -- I'm assuming you can address the two issues and update the PR even though I approved the pull request? That was a flow we often used at Facebook (approved with comments to be addressed) but idk if it works on GitHub. |
Lets do it in a new PR. |
This is curious... I expected to get some things wrong. The big reason why I took this PR is that I wanted to start learning Rust. Actually this is my first real code in Rust 😅 |
| if !info.nonPks.is_null() { | ||
| drop(Vec::from_raw_parts( | ||
| info.nonPks, | ||
| info.nonPksLen as usize, |
There was a problem hiding this comment.
I'm guessing the shrink was required for from_raw_parts to correctly drop here?
core/rs/core/src/tableinfo.rs
Outdated
| } | ||
|
|
||
| if cols.len() != columns_len { | ||
| err.set(&"Number of fetched columns did not match expected number of columns"); |
There was a problem hiding this comment.
you shouldn't need to reference the static string as it should already be a reference.
E.g.,
change
&"Number of fetched columns did not match expected number of columns"
to
"Number of fetched columns did not match expected number of columns"
haha, well I learned more ways to use generic traits and about |
core/rs/core/src/tableinfo.rs
Outdated
| drop(Box::from_raw(table_info)); | ||
| } | ||
|
|
||
| pub fn is_table_compatible(db: *mut sqlite::sqlite3, table: &str, err: *mut *mut c_char) -> bool { |
There was a problem hiding this comment.
This should return Result<bool, ResultCode> given it can fail for reasons besides the table not being compatible.
core/rs/core/src/tableinfo.rs
Outdated
| "SELECT count(*) FROM pragma_index_list('{table}') | ||
| WHERE \"origin\" != 'pk' AND \"unique\" = 1" | ||
| ); | ||
| match db.prepare_v2(&sql).and_then(|stmt| { |
There was a problem hiding this comment.
if is_table_compatible returns a Result then you can skip all the and_then and match stuff.
edit: ah we can't given we set specific error messages in every case.
There was a problem hiding this comment.
Should we treat Failed to analyze errors as Ok(false) or Err(code) then?
Could have fooled me but I took a second look over and found some things that should be addressed. |
|
@tantaman what do you think? I've added a Countable util trait since that code repeated too often. |
👍 |
Closes #325.
Functions moved to rust:
Table info is implemented with raw pointers to ensure compatibility with C counterparts. Therefore all the C tests verify the new Rust implementations.
Also we can convert the entire
tableinfo.cas long as we convertget-table.c(as it is its only dependency). I can continue doing that here or start a new issue/PR for that.