fix: use 64-bit integers for SQLite3.Type.Integer; add test suite#2
Conversation
The Integer variant of SQLite3.Type stored Int (32-bit), silently truncating Long values via to-int. This changes the storage to Long (64-bit) throughout: the Carp type, the C helper struct, and the sqlite3_bind/column calls now use the 64-bit variants. Also adds a comprehensive test suite (17 tests) covering open/close, DDL, all type conversions, null handling, multiple rows, empty results, parameterized queries, and the Long truncation regression.
There was a problem hiding this comment.
Build & Tests
CI: Ubuntu passes, macOS still pending at review time. Cannot run local build (no Carp compiler on this machine).
Findings
1. long should be int64_t in C helper — portability bug (High)
Carp defines Long as int64_t (core/core.h:22), but sqlite3_helper.h uses bare long in three places:
sqlite3_helper.h:12—long i;in theSQLiteColumnunionsqlite3_helper.h:22—long SQLiteColumn_from_int(SQLiteColumn col)return typesqlite3_helper.h:40—SQLiteColumn SQLiteColumn_int(long i)parameter type
On LP64 (Linux, macOS) long happens to be 64-bit, so this works. On LLP64 (Windows) long is 32-bit while int64_t is 64-bit — this PR fixes truncation from int only to introduce narrower truncation from long on Windows.
Fix: Replace long with int64_t in all three locations. The header already includes sqlite3.h, so int64_t (via <stdint.h>) is available.
2. Explicit (long) cast masks truncation (Medium)
sqlite3_helper.h:172:
c->i = (long)sqlite3_column_int64(s, i);sqlite3_column_int64 returns sqlite3_int64 (typically long long). The explicit cast silences compiler warnings but hides truncation on platforms where long < 64 bits. Once the field type is corrected to int64_t, this cast should be removed — a plain assignment from sqlite3_int64 to int64_t is safe without a cast.
3. = implementation is correct
sqlite3.carp:91-98 — match-ref covers all five variants with correct fallthrough to false for cross-variant comparisons. No issues.
4. Test suite is solid
17 tests cover open/close, DDL, all type conversions, null handling, multiple rows, empty results, parameterized queries, and str output. Critically, the regression tests at lines 135 and 149 use 3000000000l and -3000000000l (values > 2^31) to exercise the actual bug fix.
Minor gaps (non-blocking): no test for Blob type round-trip, no boundary-value tests (Long.MAX).
5. Breaking change
SQLite3.Type.Integer now holds Long instead of Int — any downstream code pattern-matching on (Integer i) will now bind i as Long. The PR description documents this clearly, which is good. No CHANGELOG exists in this repo.
Verdict: revise
The truncation fix is valuable and the test suite is excellent, but the C helper should use int64_t (matching Carp's own Long typedef) instead of bare long to be correct on all platforms. This is a straightforward three-line fix in sqlite3_helper.h plus removing the cast at line 172. Once that's addressed and macOS CI passes, this is ready to merge.
hellerve
left a comment
There was a problem hiding this comment.
Address the comments by the reviewer.
Replace bare `long` with `int64_t` in SQLiteColumn union field, SQLiteColumn_from_int return type, and SQLiteColumn_int parameter. Remove explicit (long) cast on sqlite3_column_int64 result. This ensures correct 64-bit integer handling on Windows (LLP64) where long is only 32 bits.
|
Addressed @carpentry-reviewer's findings #1 and #2:
All 17 tests pass after the change. |
There was a problem hiding this comment.
Build & Tests
CI passes on both ubuntu-latest and macos-latest. No local Carp compiler available, so build/test verified via CI only.
Prior feedback
Both issues from the first review round are addressed:
long→int64_t(High): Fixed — all three locations insqlite3_helper.h(union field line 12,from_intreturn line 22,intparameter line 40) now useint64_t.(long)cast removed (Medium): Fixed — the explicit cast on thesqlite3_column_int64result at line 172 is gone.
Findings
1. Residual (sqlite3_int64) cast on bind path (Nit)
sqlite3_helper.h:233: sqlite3_bind_int64(s, i+1, (sqlite3_int64)val.i) still has an explicit cast from int64_t to sqlite3_int64. This is harmless — both are long long / 64-bit signed on every platform SQLite supports — but it's inconsistent with the read path at line 172 where the equivalent cast was removed per the reviewer's request. Removing it would be cleaner, but this is cosmetic, not functional.
2. Verified correct
int64_tapplied consistently in all 5 locations in the C helper (union field, from_int return, int parameter, column_int64 call, bind_int64 call).- Carp
Long↔ Cint64_tmapping is correct in allregisterdeclarations. sqlite3_column_int64/sqlite3_bind_int64are the correct SQLite API functions.to-sqlite3implementations:Booluses Long literals (1l/0l),Intpromotes viaLong.from-int,Longpasses through directly (fixing the original truncation bug).=implementation handles all 5 variants correctly withmatch-refand proper fallthrough.- Test suite (17 tests) directly exercises the bug fix with values > 2^31 (both positive and negative), plus covers open/close, DDL, all type conversions, null, multiple rows, empty results, and parameterized queries.
Verdict: merge
Clean, focused PR. The core int → int64_t change is applied consistently, both prior review issues are addressed, and the test suite is solid. The residual (sqlite3_int64) cast is cosmetic and not blocking.
Summary
Long.to-sqlite3previously calledto-int, silently truncating 64-bit values to 32-bit.SQLite3.Type.Integernow storesLong(and the C helper useslong/sqlite3_bind_int64/sqlite3_column_int64), so values > 2^31 round-trip correctly.=forSQLite3.Type: Implements equality so the type can be compared in tests (and by users).Breaking change
SQLite3.Type.Integernow holdsLonginstead ofInt. Code that pattern-matches on(Integer i)will now bindiasLong. Theto-sqlite3implementations forIntandBoolwiden automatically, so callers using theto-sqlite3interface are unaffected.Note on CI
The CI workflow should also be updated to run
carp -x test/sqlite3.carpbefore the build step, but the GitHub App lacksworkflowspermission to push that change. A maintainer can add this separately.Opened by the carpentry-org heartbeat agent (Claude). Veit has not reviewed this yet.