From 7954849852b0998bc30b150f6c9b6f78fcb3beeb Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 12 Mar 2025 11:39:25 -0700 Subject: [PATCH] Feedback --- package.json | 2 +- src/addon.cc | 90 ++++++++++++++++++++++++++++++++-------------------- src/addon.h | 88 ++++++++++++++++++++------------------------------ 3 files changed, 91 insertions(+), 89 deletions(-) diff --git a/package.json b/package.json index ae04a16..e2db5d2 100644 --- a/package.json +++ b/package.json @@ -41,7 +41,7 @@ "build:docs": "typedoc lib/index.ts --includeVersion", "install": "node-gyp-build", "prebuildify": "prebuildify --strip --napi", - "test": "vitest --coverage", + "test": "vitest --coverage --pool threads", "format": "run-p --print-label format:c format:js", "format:c": "xcrun clang-format --style=chromium -Werror --verbose -i src/*.cc", "format:js": "prettier --cache --write .", diff --git a/src/addon.cc b/src/addon.cc index 0c05fd5..e7cacb5 100644 --- a/src/addon.cc +++ b/src/addon.cc @@ -14,9 +14,11 @@ class SignalTokenizerModule { public: - static void Destroy(void* p_ctx) {} + static void Destroy(void* p_ctx) { + delete static_cast(p_ctx); + } - inline fts5_tokenizer* GetAPIObject() { return &api_object; } + static fts5_tokenizer api_object; private: static int Create(void* p_ctx, char const**, int, Fts5Tokenizer** pp_out) { @@ -26,7 +28,6 @@ class SignalTokenizerModule { } static void Delete(Fts5Tokenizer* tokenizer) {} - static fts5_tokenizer api_object; }; fts5_tokenizer SignalTokenizerModule::api_object = { @@ -72,6 +73,27 @@ static Napi::Value SignalTokenize(const Napi::CallbackInfo& info) { return result; } +// Utils + +Napi::Error FormatError(Napi::Env env, const char* format, ...) { + va_list args; + + // Get buffer size + va_start(args, format); + auto size = vsnprintf(nullptr, 0, format, args); + va_end(args); + + // Allocate and fill the string + auto buf = new char[size + 1]; + va_start(args, format); + vsnprintf(buf, size + 1, format, args); + va_end(args); + + auto err = Napi::Error::New(env, std::string(buf, size)); + delete[] buf; + return err; +} + // Database Napi::Object Database::Init(Napi::Env env, Napi::Object exports) { @@ -126,10 +148,8 @@ Napi::Value Database::Open(const Napi::CallbackInfo& info) { sqlite3* handle = nullptr; int r = sqlite3_open_v2(path_utf8.c_str(), &handle, flags, nullptr); if (r != SQLITE_OK) { - char buf[1024]; - - snprintf(buf, sizeof(buf), "sqlite open error: %s", sqlite3_errstr(r)); - NAPI_THROW(Napi::Error::New(env, buf), Napi::Value()); + NAPI_THROW(FormatError(env, "sqlite open error: %s", sqlite3_errstr(r)), + Napi::Value()); } auto db = new Database(env, handle); @@ -145,8 +165,12 @@ Napi::Value Database::Open(const Napi::CallbackInfo& info) { return Napi::Value(); } SignalTokenizerModule* icu = new SignalTokenizerModule(); - fts5->xCreateTokenizer(fts5, "signal_tokenizer", icu, icu->GetAPIObject(), - &SignalTokenizerModule::Destroy); + r = fts5->xCreateTokenizer(fts5, "signal_tokenizer", icu, &icu->api_object, + &SignalTokenizerModule::Destroy); + if (r != SQLITE_OK) { + delete icu; + return db->ThrowSqliteError(env, r); + } return db->self_ref_.Value(); } @@ -204,20 +228,18 @@ Napi::Value Database::Exec(const Napi::CallbackInfo& info) { } Napi::Value Database::ThrowSqliteError(Napi::Env env, int error) { - char buf[1024]; - assert(handle_ != nullptr); const char* msg = sqlite3_errmsg(handle_); int offset = sqlite3_error_offset(handle_); int extended = sqlite3_extended_errcode(handle_); if (offset == -1) { - snprintf(buf, sizeof(buf), "sqlite error(%d): %s", extended, msg); + NAPI_THROW(FormatError(env, "sqlite error(%d): %s", extended, msg), + Napi::Value()); } else { - snprintf(buf, sizeof(buf), "sqlite error(%d): %s, offset: %d", extended, - msg, offset); + NAPI_THROW(FormatError(env, "sqlite error(%d): %s, offset: %d", extended, + msg, offset), + Napi::Value()); } - - NAPI_THROW(Napi::Error::New(env, buf), Napi::Value()); } fts5_api* Database::GetFTS5API(Napi::Env env) { @@ -249,7 +271,9 @@ std::list::const_iterator Database::TrackStatement( self_ref_.Ref(); statements_.emplace_back(stmt); - return --statements_.end(); + auto end = statements_.end(); + end--; + return end; } void Database::UntrackStatement(std::list::const_iterator iter) { @@ -492,8 +516,6 @@ Napi::Value Statement::Step(const Napi::CallbackInfo& info) { } bool Statement::BindParams(Napi::Env env, Napi::Value params) { - char buf[1024]; - int key_count = sqlite3_bind_parameter_count(handle_); if (params.IsNull()) { @@ -505,29 +527,29 @@ bool Statement::BindParams(Napi::Env env, Napi::Value params) { return true; } - snprintf(buf, sizeof(buf), "Expected %d parameters, got 0", key_count); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW(FormatError(env, "Expected %d parameters, got 0", key_count), + false); } else if (params.IsArray()) { auto list = params.As(); auto list_len = static_cast(list.Length()); if (list_len != key_count) { - snprintf(buf, sizeof(buf), "Expected %d parameters, got %d", key_count, - list_len); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW(FormatError(env, "Expected %d parameters, got %d", key_count, + list_len), + false); } for (int i = 1; i <= list_len; i++) { auto name = sqlite3_bind_parameter_name(handle_, i); if (name != nullptr) { - snprintf(buf, sizeof(buf), "Unexpected named param %s at %d", name, i); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW(FormatError(env, "Unexpected named param %s at %d", name, i), + false); } auto error = BindParam(env, i, list[i - 1]); if (error != nullptr) { - snprintf(buf, sizeof(buf), "Failed to bind param %d, error %s", i, - error); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW( + FormatError(env, "Failed to bind param %d, error %s", i, error), + false); } } } else { @@ -536,8 +558,8 @@ bool Statement::BindParams(Napi::Env env, Napi::Value params) { for (int i = 1; i <= key_count; i++) { auto name = sqlite3_bind_parameter_name(handle_, i); if (name == nullptr) { - snprintf(buf, sizeof(buf), "Unexpected anonymous param at %d", i); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW(FormatError(env, "Unexpected anonymous param at %d", i), + false); } // Skip "$" @@ -545,9 +567,9 @@ bool Statement::BindParams(Napi::Env env, Napi::Value params) { auto value = obj[name]; auto error = BindParam(env, i, value); if (error != nullptr) { - snprintf(buf, sizeof(buf), "Failed to bind param %s, error %s", name, - error); - NAPI_THROW(Napi::Error::New(env, buf), false); + NAPI_THROW( + FormatError(env, "Failed to bind param %s, error %s", name, error), + false); } } } diff --git a/src/addon.h b/src/addon.h index 85e3a23..425c259 100644 --- a/src/addon.h +++ b/src/addon.h @@ -79,61 +79,41 @@ class Statement { // statements. // // If return value `false` - only whitespace and comments remain. - static inline bool HasTail(const char* tail) { - for (; *tail != 0; tail++) { - switch (*tail) { - // Various whitespace - case '\t': // 0x09 - case '\r': // 0x0a - case '\v': // 0x0b - case '\f': // 0x0c - case '\n': // 0x0d - case ' ': - - // Statement separator - case ';': - break; - - // Line comment - case '-': - tail++; - if (*tail != '-') { - return true; - } - - tail = strchr(tail, '\n'); - if (tail == nullptr) { - return false; - } - - break; - + // + // Note: we use `rfind(..., 0)` for effectively a prefix check. + static inline bool HasTail(std::string const& tail) { + std::string p(tail); + while (!p.empty()) { + auto ch = p.front(); + // Various whitespace or statement separator + if (std::isspace(ch) || ch == ';') { + p = p.substr(1); + + } else if (p.rfind("--", 0) == 0) { + // Line comment: "--" + p = p.substr(2); + + // Skip until the end of the line + auto end = p.find("\n"); + if (end == p.npos) { + return false; + } + + p = p.substr(end + 1); + + } else if (p.rfind("/*", 0) == 0) { // Block comment - case '/': - tail++; - if (*tail != '*') { - return true; - } - - // Look for closing '*/' - while (true) { - tail = strchr(tail, '*'); - if (tail == nullptr) { - return false; - } - - tail++; - const char next = *tail; - if (next == 0) { - return false; - } - if (next == '/') { - tail++; - break; - } - } - default: - return true; + p = p.substr(2); + + auto end = p.find("*/"); + if (end == p.npos) { + return false; + } + + p = p.substr(end + 2); + } else { + // Not whitespace or comments + return true; } } return false;