sqlite kvstore backend #749
Conversation
arnetheduck
force-pushed
the
kvstore-sqlite
branch
from
e796891
to
35636f0
Compare
|
Works for me in my tests so far -- switching to the daemon, it |
| @@ -141,3 +141,6 @@ | |||
| url = https://github.com/status-im/nim-libbacktrace.git | |||
| ignore = dirty | |||
| branch = master | |||
| [submodule "vendor/nim-sqlite3-abi"] | |||
| path = vendor/nim-sqlite3-abi | |||
| url = https://github.com/arnetheduck/nim-sqlite3-abi.git | |||
There was a problem hiding this comment.
I think we should strive to have all submodules pointing to repos in the status-im org. Otherwise, dealing with any discovered issues becomes harder.
|
|
||
| func raiseError(err: cint) {.noreturn.} = | ||
| let tmp = sqlite3_errstr(err) | ||
| raise (ref SqliteError)(msg: $tmp) |
There was a problem hiding this comment.
You could have avoided this awkward syntax if SqliteError was defined as ref oject of CatchableError and you used raise newException(SqliteError, $tmp) here.
There was a problem hiding this comment.
hold on..
exceptions should be non-ref:
https://nim-lang.org/docs/manual.html#exception-handling-custom-exceptions
(manual recommends using Exception as base class btw).
newException turns non-ref exception type into ref - that's why it exists, no?
https://github.com/nim-lang/Nim/blob/devel/lib/system.nim#L1948
one of the often touted reasons to use exceptions is to include additional data - newException prevents you from doing this elegantly - you have to create a partially initialized object then fill it in (somewhat irrelevantly, newException itself is poorly implemented in this regard - it first allocates the exception then fills it in piece by piece)
this syntax is shorter, clearly states that a ref is raised instead of hiding it in a template (ie that heap memory is involved and all the other ref-counting issues) and provides access to all the fields of the error object - seems like a win?
There was a problem hiding this comment.
Hrm, right.
I guess I have to get used to that syntax until something better comes along.
There was a problem hiding this comment.
hahaha: nim-lang/Nim#13373
| insertStmt = prepare "INSERT OR REPLACE INTO kvstore(key, value) VALUES (?, ?);": | ||
| discard sqlite3_finalize(selectStmt) | ||
| deleteStmt = prepare "DELETE FROM kvstore WHERE key = ?;": | ||
| discard sqlite3_finalize(selectStmt) |
There was a problem hiding this comment.
This was a good opportunity to try out Nim's destructors ;)
There was a problem hiding this comment.
we'd need move support as well, because you'd have to disarm the source when copying the held c pointer, and a way to prevent spurious copies (ie delete copy constructor in C++).. is that part of the package?
There was a problem hiding this comment.
The move support would be an additional simplification. For now, we can easily stick to release() procs in the style of std::auto_ptr/unique_ptr.
| case v | ||
| of SQLITE_ROW: | ||
| let | ||
| p = cast[ptr UncheckedArray[byte]](sqlite3_column_blob(db.selectStmt, 0)) |
There was a problem hiding this comment.
It would have been nice if the sqlite3_column_blob function was returning ptr UncheckedArray[byte]. That's a more accurate description of the result and no casting will be necessary here.
There was a problem hiding this comment.
blob returns a void* though - it's technically not an ptr UncheckedArray[byte]*, the byte part is not quite true - though I guess there's no practical difference. it would require manually adjusting what the wrapper generates - once you start doing that, there's a lot you could improve here (ie ptr sqlite3 is not very nim:ish either)
There was a problem hiding this comment.
Well, this could justify the dependency on a fancier tool such as Nimterop for the purposes of creating the wrapper. I could imagine adding a single rule that changes the return type of all functions ending with "_blob" here:
https://github.com/arnetheduck/nim-sqlite3-abi/blob/master/wrap.nim#L22
No description provided.