sqlite3_busy_handler#598
sqlite3_busy_handler#598undisputed-seraphim wants to merge 2 commits intofnc12:devfrom undisputed-seraphim:busy_handler
Conversation
|
hi. Please make code format. One can use this command |
|
Woops, done |
dev/storage_base.h
Outdated
| * must not go out of scope for the lifetime of the database. | ||
| */ | ||
| template<typename T = void> | ||
| int busy_handler(std::function<int(T *, int)> f, T *ptr) { |
There was a problem hiding this comment.
why do you pass a pointer? Pointer means that value can be null. What if it is null? Then line 139 will lead to undefined behavior. IMHO it is better to pass a reference in public API and bind it as a pointer inside.
| auto h = *reinterpret_cast<handler_t *>(ptr); | ||
| return h.fn(h.ptr, times); | ||
| }, | ||
| &handler); |
There was a problem hiding this comment.
handler will be destroyed once this function exits. Then its' address will be captured as user data. Then dereferencing it will lead to UB. It is got to be fixed.
| * must not go out of scope for the lifetime of the database. | ||
| */ | ||
| template<typename T = void> | ||
| int busy_handler(std::function<int(T *, int)> f, T &data) { |
There was a problem hiding this comment.
I don't understand why do you need this function to be template. Actually std::function allows us binding any data we want so it is enough. Dedicated argument for custom data is not required - it can be stored inside lambdas capture. std::function must be a member inside storage_base. Next in on_open_internal you must call sqlite3_busy_handler if this member is not null. Custom data argument must be a pointer to a storage and callback that is must be passed to sqlite3_busy_handler function must be a static private/protected function in storage_base class. Why this idea is better? Cause it has less templates. Templates must exist only if we cannot avoid them. The second is custom data is stored inside std::function and this is ok - it solves the issue from the first comment. I'd like you to refactored your solution as I described here. Of course if you have a different opinion or a better solution please feel free to comment it here or email me (the first option is more preferred cause conversation will be available for everyone). If you want I can implement it on my own today or tomorrow.
There was a problem hiding this comment.
I have chosen to write it this way for one simple reason: Because it directly correlates to the function signature documented at sqlite, int(void*, int).
The user of this interface may check sqlite3_busy_handler, see that it asks for a function int(void*, int), and see that this interface does not expose that functionality. It is not obvious that the idea is to capture data in the lambdas.
On the other hand, it is clear to the user here that we are simply imposing stronger types on top of the original int(*)(void*,int).
I have seen your merge request. It is good, but please add documentation about the intention.
|
@undisputed-seraphim please check out this branch #600. I need feedback from you whether it has functionality you want. Thanks |
|
@undisputed-seraphim are you there? |
|
Hi sorry for the silence. Life got busy suddenly. I'll take a look. |
2 participants
I propose a possible implementation of a busy_handler function.
As sqlite3 has a pure C interface, it is not possible to simply pass
std::functionor lambdas intosqlite3_busy_handler().My solution is to shove a
std::functionand the user pointer into a struct, pass it into the C interface as a void pointer, and unpack it from the inside.This way, we get to also expose a strongly typed interface to the user, through the template.
It can accept lambda captures too.
Warning: I don't really have a way to unit test this. The tests written are to ensure compile-ability only.