I'm using this nice C++ wrapper to work with SQLite3 in my project. I wanted to handle errors gracefully and automatically rollback if piece of code fails, so wrote this simple helper class:
class SqlTransaction { // RAII class to rollback on error
public:
SqlTransaction(std::shared_ptr<sqlite::database> db_): db(db_) { *db << "begin;"; };
~SqlTransaction() { if (!finished) rollback(); };
void commit() { if (!finished) { *db << "commit;"; finished = true; } };
void rollback() { if (!finished) { *db << "rollback;"; finished = true; } };
// Disable both copying and moving
SqlTransaction(const SqlTransaction&) = delete;
SqlTransaction& operator=(const SqlTransaction&) = delete;
SqlTransaction(SqlTransaction&&) = delete;
private:
std::shared_ptr<sqlite::database> db;
bool finished = false;
};
It's to be used like this:
SqlTransaction trans(db);
// work with db here, may throw if something goes wrong
trans.commit();
2 Answers
There's not a lot of code here to be reviewed, but I'll have a go.
A small efficiency gain is possible, by moving the
db_argument in the initializer list, rather than copying it:SqlTransaction(std::shared_ptr<sqlite::database> db_) : db{std::move(db_)} { *db << "begin;"; };It's not necessary to delete the move constructor, as explicitly deleting the copy constructor prevents the move constructor being implicitly provided. However, if you feel that doing so improves clarity, you should probably delete move assignment, too.
We should document that the class isn't thread-safe. It's quite reasonable that we should use it from only one thread, but we need to be clear to our users about that. Alternatively, we could make it thread-safe by making
finishedanstd::atomic<bool>and using its test-and-set method,exchange().Since
rollback()testsfinished, there's no need to duplicate that in the destructor - just callrollback()unconditionally. It may be worth taking steps to avoid it throwing when called there - destructors that throw need to be handled with extreme care.~SqlTransaction() { try { rollback(); } catch (...) { /* ignore */ } };
I have nothing specific to SQLite, as I've not used that library myself (I'm surprised it has to parse string commands, rather than having methods for those operations, though).
It's a good idea to encapsulate transaction-management.
You don't go anywhere far enough though. Use std::uncaught_exceptions() to automate it more. (Before C++17, you have to use some non-standard ways to get that count.)
Also, while rolling back must never fail, committing may. So, mark noexcept and noexcept(false) as appropriate.
Last but not least, while copying a std::shared_ptr is not really expensive, moving it is much cheaper still.
class SqlTransaction {
public:
SqlTransaction(std::shared_ptr<sqlite::database> db_)
: db(std::move(db_))
{ *db << "begin;"; }
~SqlTransaction() noexcept(false) {
auto current = std::uncaught_exceptions();
if (count == current)
do_commit();
else if (count < current)
do_rollback();
}
void commit() {
if (std::exchange(count, max_count) != max_count)
do_commit();
}
void rollback() noexcept {
if (std::exchange(count, max_count) != max_count)
do_rollback();
}
private:
void do_commit() {
*db << "commit;";
}
void do_rollback() noexcept {
*db << "rollback;";
}
SqlTransaction(const SqlTransaction&) = delete;
SqlTransaction& operator=(const SqlTransaction&) = delete;
std::shared_ptr<sqlite::database> db;
int count = std::uncaught_exceptions();
static constexpr max_count = std::numeric_limits<int>::max();
};
