fix: address critical and important review findings#2
Merged
Conversation
Critical:
- Wrap Document.scratch in RefCell<Vec<u8>> so qjd_get_str /
qjd_cursor_get_str no longer alias &Document with &mut scratch
(Stacked Borrows UB; Miri would flag).
- Guard the 5 unit tests in src/scan/avx2.rs with a runtime
is_x86_feature_detected check so `cargo test` does not SIGILL on
x86_64 hosts without AVX2/PCLMUL.
Important:
- Add `avx2` Cargo feature (default-on); gate the AVX2 module,
dispatcher and proptest behind it so `cargo test --no-default-features`
exercises the scalar-only path per spec §9.4. CI runs both.
- Rename qjd_cursor.cache_slot / _pad to _reserved0 / _reserved1 in
the Rust struct, C header, Lua cdef and spec — the slot is never
read in v1, so the old name misled C consumers.
- Store child_ends alongside child_starts in SkipSlot so the cache-hit
path in resolve_in_known_children skips find_value_span entirely,
matching spec §6.3 "no brace counting".
- Reuse the module-level cur_box in Lua Cursor:open/field/index
instead of ffi.new("qjd_cursor[1]") per call — keeps LuaJIT traces
intact on the hot path.
- Update README "Status" to reflect the shipped implementation.
- Add spec §9.2 coverage: 5K-key wide object cache test, i64::MAX/MIN
and 1.7e308 boundaries, i64-overflow at 2^63, float-form rejection
for get_i64, Lua Doc GC finalizer test.
Minor:
- Drop unused #[allow(dead_code)] attributes; mark test-only
SkipCache::len with #[cfg(test)].
- Fix spec module-layout diagram to match the actual tree (no
runtime_dispatch.rs; cursor.rs / path.rs / error.rs exist).
- Note missing large_dump.json / deep_nest.json bench fixtures in
README Roadmap.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Document.scratchinRefCellto eliminate&Document+&mut scratchaliasing UB inqjd_get_str/qjd_cursor_get_str; runtime-guard the 5 AVX2 unit tests socargo testno longer SIGILLs on non-AVX2 hosts.avx2Cargo feature (default-on) + CI step for--no-default-features; rename misleadingqjd_cursor.cache_slotto_reserved0/1; storechild_endsinSkipSlotso cache-hit lookups skipfind_value_span(spec §6.3); reuse module-levelcur_boxin LuaCursor:open/field/index; update README status; add §9.2 tests (5K-key wide object, i64/f64 boundaries, Lua Doc GC).Test plan