Skip to content

Review feedback #1

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.

Sign up for GitHub

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 1 commit into from
Mar 12, 2025
Merged

Review feedback #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 .",
Expand Down
90 changes: 56 additions & 34 deletions src/addon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@

class SignalTokenizerModule {
public:
static void Destroy(void* p_ctx) {}
static void Destroy(void* p_ctx) {
delete static_cast<SignalTokenizerModule*>(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) {
Expand All @@ -26,7 +28,6 @@ class SignalTokenizerModule {
}

static void Delete(Fts5Tokenizer* tokenizer) {}
static fts5_tokenizer api_object;
};

fts5_tokenizer SignalTokenizerModule::api_object = {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand All @@ -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();
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -249,7 +271,9 @@ std::list<Statement*>::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<Statement*>::const_iterator iter) {
Expand Down Expand Up @@ -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()) {
Expand All @@ -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<Napi::Array>();
auto list_len = static_cast<int>(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 {
Expand All @@ -536,18 +558,18 @@ 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 "$"
name = name + 1;
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);
}
}
}
Expand Down
88 changes: 34 additions & 54 deletions src/addon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading