Skip to content

Implement Sqlite3 Module #4260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 74 commits into from
Jan 7, 2023
Merged

Implement Sqlite3 Module #4260

merged 74 commits into from
Jan 7, 2023

Conversation

qingshi163
Copy link
Contributor

@qingshi163 qingshi163 commented Nov 4, 2022

closes #3983

@qingshi163 qingshi163 force-pushed the sqlite branch 3 times, most recently from 0a7f6fe to 8bebe38 Compare December 22, 2022 14:01
@qingshi163 qingshi163 changed the title [WIP] Implement Sqlite3 Module Implement Sqlite3 Module Jan 1, 2023
@qingshi163
Copy link
Contributor Author

@youknowone Can you help with that compilation fail with android?

@qingshi163 qingshi163 requested a review from youknowone January 1, 2023 15:28
@qingshi163 qingshi163 marked this pull request as ready for review January 3, 2023 11:34
@youknowone
Copy link
Member

that looks like a toolchain problem. rather than fighting with it, how about just disabling sqlite3 for android?

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! You made a huge work!

Comment on lines +846 to +856
let guard = self.db.lock();
if guard.is_some() {
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))
} else {
Err(new_programming_error(
vm,
"Cannot operate on a closed database.".to_owned(),
))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let guard = self.db.lock();
if guard.is_some() {
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))
} else {
Err(new_programming_error(
vm,
"Cannot operate on a closed database.".to_owned(),
))
}
let Some(guard) = self.db.lock() else {
return Err(new_programming_error(
vm,
"Cannot operate on a closed database.".to_owned(),
));
};
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work as the variable guard is MutexGuard<Option<_>>

Comment on lines 1097 to 1099
unsafe {
sqlite3_create_collation_v2(db.db, name.as_ptr(), SQLITE_UTF8, null_mut(), None, None);
return Ok(());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unsafe {
sqlite3_create_collation_v2(db.db, name.as_ptr(), SQLITE_UTF8, null_mut(), None, None);
return Ok(());
}
unsafe {
sqlite3_create_collation_v2(db.db, name.as_ptr(), SQLITE_UTF8, null_mut(), None, None);
}
return Ok(());

later code's unsafe blocks don't contain return Ok(()) but this one does.

Comment on lines +1365 to +1375
let guard = self.inner.lock();
if guard.is_some() {
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))
} else {
Err(new_programming_error(
vm,
"Cannot operate on a closed cursor.".to_owned(),
))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let guard = self.inner.lock();
if guard.is_some() {
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))
} else {
Err(new_programming_error(
vm,
"Cannot operate on a closed cursor.".to_owned(),
))
}
let Some(guard) = self.inner.lock() else {
return Err(new_programming_error(
vm,
"Cannot operate on a closed cursor.".to_owned(),
));
};
Ok(PyMutexGuard::map(guard, |x| unsafe {
x.as_mut().unwrap_unchecked()
}))

}

fn inner(&self, vm: &VirtualMachine) -> PyResult<PyMappedMutexGuard<BlobInner>> {
let guard = self.inner.lock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let-else for guard here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do let-else on a MutexGuard?

@qingshi163 qingshi163 merged commit 7659331 into RustPython:main Jan 7, 2023
@qingshi163 qingshi163 deleted the sqlite branch January 7, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding sqlite
3 participants