Code Review 3

Author: SRombauts

CODE_REVIEW.md

@@ -0,0 +1,44 @@
+# Findings — 3rd pass: Backup + Exception
+
+_Scope: `include/SQLiteCpp/Backup.h`, `src/Backup.cpp`, `include/SQLiteCpp/Exception.h`, `src/Exception.cpp`, `tests/Backup_test.cpp`, `tests/Exception_test.cpp`. Evidence checked against the bundled amalgamation `sqlite3/sqlite3.c` (backup_init at 84859, backup_step at 85033, sqlite3ErrStr at 188846)._
+
+## 1. Fix verification: EXC-02 (#552)
+
+**Complete.** `src/Exception.cpp:18-19` now reads `std::runtime_error(aErrorMessage ? aErrorMessage : "")`; both public `const char*` entry points (`Exception.h:35,47`) route through this master constructor, and the `std::string` overloads cannot pass null. A regression test exists at `tests/Exception_test.cpp:89-95` (`Exception(static_cast<const char*>(nullptr), 1)` → `what() == ""`, codes preserved). No remaining null-message path into `std::runtime_error` in this unit.
+
+## 2. Status of still-open findings
+
+- **BKP-01** — Still valid: `Backup.h:113` has no nodiscard; `src/Database.cpp:376` still calls `bkp.executeStep();` and drops the return (BUSY/LOCKED → silent partial backup).
+- **BKP-02** — Still valid: `Backup.h:97-98` deleted copies suppress implicit moves; no move ops declared; class remains non-movable.
+- **BKP-03** — Still valid: `Backup.h:116,119` getters still lack `noexcept`/nodiscard.
+- **BKP-04** — Still valid but the **proposed fix must change**: verified in `sqlite3.c:85033-85290` that `sqlite3_backup_step` stores its error only in `p->rc` and returns it — it does **not** post the error to the destination connection. Building the step exception from the dest handle (the fix BKP-04 proposed) would read *stale* connection state. The correct improvement is BKP-08 below (split `res` into primary+extended).
+- **BKP-05** — Still valid: no `Database` reference/handle member retained; lifetime contract still undocumented in `Backup.h`.
+- **BKP-06** — Still a verified non-issue: `Deleter` (`src/Backup.cpp:75-81`) correctly swallows `sqlite3_backup_finish`'s code; null-guard redundant-but-harmless (`sqlite3_backup_finish(NULL)` returns `SQLITE_OK`, `sqlite3.c:85296`).
+- **BKP-07** — Still valid: counts before the first `executeStep()` return 0 (`p->nPagecount`/`nRemaining` only set inside step, `sqlite3.c:85121-85124`); still undocumented in `Backup.h:115-119`.
+- **EXC-01** — Still valid: `src/Exception.cpp:21` hard-codes `mExtendedErrcode(-1)` in the `(const char*, int)` ctor; tests still assert `-1` (`Exception_test.cpp:39,65,72,79,86`). Aggravated by BKP-08 below.
+- **EXC-03** — Still valid: no `SQLITECPP_NODISCARD` macro exists anywhere; accessors at `Exception.h:72,78,84` unannotated.
+- **EXC-04** — Still valid: `getErrorStr()` doc (`Exception.h:83`) still silent on the static-storage lifetime of the returned pointer.
+- **EXC-05** — Still valid: `Exception.h:87-88` members non-`const` with no comment explaining the copy-assignability rationale.
+- **EXC-06** — Still valid: `tests/Exception_test.cpp:2` still headed `Transaction_test.cpp`, "avaiable" typo at line 28; stray-indent `/**` at `Exception.h:56`.
+- **EXC-07** — Still valid: `-1` sentinel duplicated at `Exception.h:48,52`.
+
+## 3. New findings
+
+| Done | ID | Severity | Confidence | Category | Location | Impact | Proposed fix |
+|:--:|----|----------|------------|----------|----------|--------|--------------|
+| [ ] | BKP-08 | Medium | High | bug | `src/Backup.cpp:57` (with `src/Exception.cpp:18-23`) | `sqlite3_backup_step` returns **extended** codes for fatal I/O errors — `sqlite3.h:9965-9967` explicitly lists "`SQLITE_IOERR_ACCESS \| SQLITE_IOERR_XXX`" as return values, and `backupOnePage`/pager errors propagate unmasked (`sqlite3.c:85109-85120`, `return rc` at 85284 with no `& 0xff`). The throw `Exception(sqlite3_errstr(res), res)` therefore stores an extended code (e.g. `SQLITE_IOERR_WRITE`=778) in `mErrcode` while `mExtendedErrcode` stays `-1` — the two getters' semantics are **inverted**: `getErrorCode()==778` (not a primary code, so `ex.getErrorCode()==SQLITE_IOERR` comparisons fail) and `getExtendedErrorCode()==-1` (the one place an extended code is genuinely available, it is dropped). Message is unaffected (`sqlite3ErrStr` masks `rc &= 0xff`, `sqlite3.c:188897`, so 778 → "disk I/O error"). | In `executeStep()` throw with the primary code in the primary slot: `throw SQLite::Exception(sqlite3_errstr(res), res & 0xFF);` and preserve the extended code — either via the EXC-01 fix (seed `mExtendedErrcode` from the ctor's `ret`, passing full `res`) or by adding an `Exception(const char*, int primary, int extended)` overload used here. Add a test asserting code slots for a fatal step error (the existing `executeStepException` readonly case returns `SQLITE_READONLY`=8, which happens to be primary — extend it to check `getErrorCode()` is a primary code). |
+| [ ] | BKP-09 | Low | High | api/doc | `include/SQLiteCpp/Backup.h:107,113`; `src/Backup.cpp:52-54` | `executeStep(0)` is a silent no-op: the copy loop `for(ii=0; (nPage<0 || ii<nPage) && ...` (`sqlite3.c:85109`) copies zero pages when `nPage==0`, and with pages remaining `rc` stays `SQLITE_OK` — never `SQLITE_DONE`. A retry loop `while (SQLITE_DONE != backup.executeStep(n))` with a caller-computed `n` that reaches 0 spins forever making no progress. The doc comment only defines negative ("all remaining") and implies any other value copies pages; 0 is undocumented. | Either document "aNumPage == 0 copies no pages and cannot return SQLITE_DONE" in the Doxygen at `Backup.h:107`, or normalize in the wrapper (`if (aNumPage == 0) aNumPage = -1;`) / reject via `SQLITECPP_ASSERT`. Documenting is the non-breaking option. |
+| [ ] | EXC-08 | Info | High | bug (edge) | `include/SQLiteCpp/Exception.h:37-40,51-54` | The `std::string` overloads delegate through `.c_str()` into the `const char*` master ctor, so a message containing an embedded NUL is silently truncated at the first NUL even though `std::runtime_error(const std::string&)` would preserve the full content. Cosmetic loss of diagnostic text only; no memory error. (Same pattern class as SP-04.) | Invert the delegation: make the master ctor `Exception(const std::string&, int)` constructing `std::runtime_error(aErrorMessage)` directly, and have the `const char*` overloads delegate with `std::string(apMsg ? apMsg : "")`. Preserves the EXC-02 null guard and removes the truncation. |
+| [ ] | BKP-10 | Info | High | style | `src/Backup.cpp:75` | `Deleter::operator()` is defined as `void SQLite::Backup::Deleter::operator()(...)` *inside* `namespace SQLite` — a redundantly qualified member definition, inconsistent with every other definition in the file (`Backup::Backup`, `Backup::executeStep` are unqualified). Legal C++ and compiles warning-clean, but it reads as a stray copy from outside the namespace. | Drop the `SQLite::` prefix: `void Backup::Deleter::operator()(sqlite3_backup* apBackup)`. |
+
+## 4. Verified non-issues (hunt list)
+
+- **`sqlite3_backup_init` error read from the right handle — CORRECT.** All three NULL-return paths in `sqlite3_backup_init` post the error to the **destination** connection: same-connection (`sqlite3ErrorWithMsg(pDestDb, SQLITE_ERROR, "source and destination must be distinct")`, `sqlite3.c:84885-84888`), OOM (`sqlite3Error(pDestDb, SQLITE_NOMEM_BKPT)`, 84897), and bad-name/open-txn (`findBtree`/`checkReadTransaction` "error has already been written into the pDestDb handle", 84911-84921). `src/Backup.cpp:34` throws `Exception(aDestDatabase.getHandle())` — matches exactly, and the read happens immediately after `reset()` with no intervening dest-handle calls. Covered by `Backup_test.cpp:24-38` (all three constructor forms).
+- **Backup between the same connection** fails cleanly at init (`pSrcDb==pDestDb` check above) with a clear message; tested. Two *different* connections to the same file is legal per SQLite (first step takes the exclusive dest lock; contention surfaces as `SQLITE_BUSY` in the return value — subject to BKP-01 if the caller drops it), not a wrapper defect.
+- **`Exception` copy / slicing — safe.** Members are two `int`s; the implicitly-generated copy/move/assign are correct, and `std::runtime_error`'s copy is `noexcept` with `what()` guaranteed equal after copy. Exercised by `Exception_test.cpp:18-41`. Catching by `std::runtime_error&`/`std::exception&` keeps `what()` (only the code getters are sliced away) — standard, unavoidable, fine.
+- **`what()` lifetime — safe.** The message is copied into the `std::runtime_error` base at construction (before SQLite's transient `errmsg` buffer can be overwritten); no `const char*` is stored in `Exception` itself.
+- **`what()` / accessor `noexcept` — sound.** `what()` is inherited `noexcept`; `getErrorCode`/`getExtendedErrorCode` are trivial; `getErrorStr()` calls only `sqlite3_errstr` (C function, cannot throw; never returns NULL — default `"unknown error"`). `sqlite3_errstr(-1)` is defined: `-1 & 0xff = 255` ≥ `ArraySize(aMsg)` → `"unknown error"` (asserted by `Exception_test.cpp:66`).
+- **`std::string` vs `const char*` ctor overload set — no ambiguity.** String literals and `char*` bind to the `const char*` overloads; `std::string` lvalues to the `const std::string&` ones; no implicit-conversion trap (only the EXC-08 NUL-truncation nit).
+- **`SQLITE_OK == 0` assumptions — safe.** The `-1` sentinel comments (`Exception.h:48,52`) and `SQLite::OK` usage rely on `SQLITE_OK==0`, which is a frozen SQLite API guarantee; `executeStep()` compares against named constants, not `0`/truthiness.
+- **Extended BUSY/LOCKED variants do not create a false-fatal in the wrapper.** `sqlite3_backup_step` generates `SQLITE_BUSY` directly as the primary code (`sqlite3.c:85060`), and `isFatalError()` (`sqlite3.c:84937-84939`) means any extended code that reaches `p->rc` is fatal *to SQLite itself* (subsequent steps fail too) — so the wrapper throwing on codes outside {OK, DONE, BUSY, LOCKED} matches SQLite's own retriability classification. Only the code-slot placement is wrong (BKP-08), not the throw decision.
+- **`getRemainingPageCount()`/`getTotalPageCount()` can never see a NULL handle** post-construction (ctor throws on NULL init; class is non-movable so no moved-from state).

CODE_REVIEW_3.md

@@ -0,0 +1,38 @@
+# Build / CI / Examples / Tests — Findings, 3rd independent review pass
+
+_Date: 2026-07-02. Scope: CMakeLists.txt, sqlite3/CMakeLists.txt, cmake/, meson.build, .github/workflows/, appveyor.yml, package.xml, examples/, tests/ (hygiene/portability). Reviewed directly against the current tree._
+
+## 1. Fix verification
+
+| ID | Claimed fix | Verified in current code |
+|----|-------------|--------------------------|
+| BLD-001 | #560 / a6537fe | **Fixed.** `meson.build:132` now appends `-DSQLITECPP_DISABLE_STD_FILESYSTEM` to `sqlitecpp_args` (the undeclared `sqlitecpp_cxx_flags` is gone; the whole file consistently uses `sqlitecpp_args`). |
+
+## 2. Status of still-open items
+
+- **BLD-002** — Still valid: googletest submodule unchanged (stale 2018 pin vs Meson `gtest.wrap`).
+- **BLD-003** — Still valid: all workflows use mutable tags (`actions/checkout@v4` in cmake.yml:56 etc.); no `permissions:` block in any of the 6 workflows.
+- **BLD-004** — Still valid: `.travis.yml` gone from root? No — `appveyor.yml` still present with retired images; Travis file removed earlier but token rotation not evidenced. Treat as open until confirmed.
+- **BLD-005** — Still valid, and broader than recorded: **none** of the 6 workflows (cmake.yml, cmake_builtin_lib.yml, cmake_subdir_example.yml, coverage.yml, coverity.yml, meson.yml) has a `permissions:` block.
+- **BLD-006** — Still valid: `SQLITECPP_USE_ASAN` unchanged; no sanitizer CI job. (Note: 3rd-pass verification ran ASan/UBSan manually on Linux — clean; see `verification_3.md`.)
+- **BLD-007 / BLD-008** — Still valid: `examples/example2/CMakeLists.txt` `CACHE BOOL "" FORCE` unchanged; `sqlite3/CMakeLists.txt:16` still uses directory-global `add_definitions("-DSQLITE_API=__declspec(dllexport)")` (export-only).
+- **EXM-001 / EXM-002 / EXM-003** — Still valid: no commits touched `examples/` since the 2nd pass (`git diff 5fa55c6..HEAD -- examples` is empty).
+- **TST-001** — Still valid: tests still use `[[maybe_unused]]` while the CMake default is C++11 (`CMakeLists.txt:12-13`).
+
+## 3. NEW findings
+
+| Done | ID | Severity | Confidence | Category | Location | Finding / Impact | Concrete fix |
+|:--:|----|----------|------------|----------|----------|------------------|--------------|
+| [ ] | BLD-009 | Low | High | build / packaging | `sqlite3/CMakeLists.txt:72-79` | `install(TARGETS sqlite3 ...)` + `install(FILES sqlite3.h DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})` run **unconditionally** whenever the internal SQLite is built — they ignore `SQLITECPP_INSTALL=OFF`, and installing a bare `sqlite3.h` into the include root can shadow or clash with a system SQLite of a different version for every other package on the prefix. | Gate the sqlite3 install rules on `SQLITECPP_INSTALL`, and consider installing the header under a subdirectory (or not at all, since the wrapper's public headers only need the sqlite3 target at link time when `SQLITE_HAS_CODEC`/legacy-struct options are off). |
+| [ ] | BLD-010 | Low | High | build / packaging | `CMakeLists.txt:388`, `cmake/SQLiteCppConfig.cmake.in:13` | `install(EXPORT SQLiteCppTargets ...)` has **no `NAMESPACE`**, so consumers of `find_package(SQLiteCpp)` get bare global target names `SQLiteCpp` and `sqlite3` instead of the conventional `SQLiteCpp::SQLiteCpp`. A consumer that also defines/imports a target named `sqlite3` (very common) collides. | Add `NAMESPACE SQLiteCpp::` to the `install(EXPORT)` and (for compatibility) an `add_library(SQLiteCpp ALIAS ...)` shim note in the Config file; document the migration. Breaking for existing consumers — flag in CHANGELOG. |
+| [ ] | BLD-011 | Low | Medium | build / ABI | `CMakeLists.txt:538` | `SOVERSION` is hardcoded `0` with no ABI policy while the 3.x series changes public headers (e.g. `Header` struct fields in #558 changed layout/types). Shared-library consumers on Linux get silent ABI drift under an unchanged soname `libSQLiteCpp.so.0`. | Either bump SOVERSION on ABI-breaking releases (document the policy) or set it from `${PROJECT_VERSION_MAJOR}`. |
+| [ ] | BLD-012 | Info | High | build / consistency | `meson.build:4-7` vs `CMakeLists.txt:12-16` | The two build systems compile different language modes by default: Meson forces `cpp_std=c++17, warning_level=3`; CMake defaults to C++11 with a hand-rolled warning list. CI therefore never builds the C++11 mode with Meson nor C++17 with the default CMake flags — masking standard-dependent issues (TST-001 is invisible to the Meson CI). | Align the defaults, or add one CI job per standard per build system (a small matrix axis). |
+| [ ] | TST-002 | Info | Medium | tests / hygiene | `tests/*.cpp` (e.g. `Database_test.cpp:50,79,91`; 44 uses of `"test.db3"`, plus `backup_test.db3`, `short.db3`, ...) | All suites hardcode relative db filenames in the CWD and delete them with `remove()` on the success path only. A mid-test failure/abort leaves stale db files that can poison subsequent runs (tests that assume absence/CREATE), and two test processes cannot run from the same directory. | Use per-test unique names (gtest `UnitTest::GetInstance()->current_test_info()`) or a RAII temp-file fixture that removes in `TearDown()`; at minimum `remove()` in SetUp too. |
+| [ ] | TST-003 | Info | High | tests / warnings | `tests/Database_test.cpp:551` | `EXPECT_EQ(h.userVersion, 12345)` compares unsigned field vs signed literal → `-Wsign-compare`, the **only** warning in an otherwise clean `-Wall -Wextra -Wpedantic -Wshadow -Wswitch-enum` build of lib+tests+examples (see `verification_3.md`). Breaks `-Werror` test builds. | `12345u` (and matching literal for the field's fixed-width type). |
+
+## 4. Verified non-issues
+
+- **Internal-sqlite export is complete** (looked wrong, is correct): `SQLiteCpp` links `PUBLIC SQLite::SQLite3` (alias of internal `sqlite3`), and `sqlite3/CMakeLists.txt:73` adds `sqlite3` to the *same* `SQLiteCppTargets` export set, so `install(EXPORT)` generates without error and consumers resolve the dependency; `SQLiteCppConfig.cmake.in` correctly `find_dependency(SQLite3)` only when `NOT SQLITECPP_INTERNAL_SQLITE`, plus `Threads` on UNIX.
+- **Version consistency**: CMake `project(SQLiteCpp VERSION 3.3.3)`, `meson.build version: '3.3.3'`, `package.xml <version>3.3.3</version>` all agree (only the `SQLITECPP_VERSION` *string* `"3.03.03"` is non-canonical — already HDR-07).
+- **CMake C++ standard guard** is correct: respects a user-provided `CMAKE_CXX_STANDARD`, defaults 11, warns below 11, `CMAKE_CXX_STANDARD_REQUIRED ON`.
+- **`SQLITE_HAS_CODEC` + internal SQLite** is a clean `FATAL_ERROR` (CMakeLists.txt:285-287) — the unsupported combination cannot be misconfigured silently.