vrischmann / zig-sqlite

zig-sqlite is a small wrapper around sqlite's C API, making it easier to use with Zig.
MIT License
367 stars 49 forks source link

Possible improvement on errorFromResultCode #92

Closed jiacai2050 closed 2 years ago

jiacai2050 commented 2 years ago

https://github.com/vrischmann/zig-sqlite/blob/9d011583eaf01be6bdc8bd3b777fd7e3e2bbf572/errors.zig#L132

I notice this comment when randomly read code, I think a possible way to use comptime here:

@@ -128,12 +128,17 @@ pub const Error = SQLiteError ||
     SQLiteExtendedReadOnlyError ||
     SQLiteExtendedConstraintError;

+fn sqliteVersionLargeThan(actual: i32) bool {
+    return c.SQLITE_VERSION_NUMBER >= actual;
+}
+
 pub fn errorFromResultCode(code: c_int) Error {
     // TODO(vincent): can we do something with comptime here ?
     // The version number is always static and defined by sqlite.

     // These errors are only available since 3.25.0.
-    if (c.SQLITE_VERSION_NUMBER >= 3025000) {
+    // or just   const is_gt_3025000 = c.SQLITE_VERSION_NUMBER >= 3025000;
+    if (comptime sqliteVersionLargeThan(3025000)) {

What do you think?

jiacai2050 commented 2 years ago

Since both c.SQLITE_VERSION_NUMBER and 3025000 are number literals, LLVM may also eliminate the if condition even without comptime ...

vrischmann commented 2 years ago

Hi,

thanks for the suggestion. Sorry I haven't had time to look into this yet.

Something like you're suggesting would work I think but I actually want to test this with old SQLite versions.

vrischmann commented 2 years ago

So, I did some test which resulted in #93. I forgot that with const values we don't need to add comptime so the TODO is obsolete: it's already doing comptime checks.

I don't think your suggested sqliteVersionLargeThan function is that useful as is. However a function like

fn sqliteVersionLargerThan(major: u8, minor: u8, patch: u8) bool

would be nice.

jiacai2050 commented 2 years ago

Here is it

fn sqliteVersionLargeThan(major: u8, minor: u8, patch: u8) bool {
    return c.SQLITE_VERSION_NUMBER >= @as(u32, major) * 1000000 + @as(u32, minor) * 1000 + @as(u32, patch);
}
vrischmann commented 2 years ago

Would you like to make a PR for this ?