Fix present/link sync regressions and harden framebuffer integrity#103
Merged
RtlZeroMemory merged 10 commits intomainfrom Feb 27, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds C++ compatibility through extern "C" guards across multiple public headers, introduces hyperlink payload comparison and target equality checks in diff logic, implements an in-place submit rollback mechanism via framebuffer synchronization tracking, and enhances framebuffer link handling with cloning, compaction, and validation logic. Additional test coverage validates damage resync with hyperlinks and wide-glyph bounds safety. Changes
Sequence DiagramsequenceDiagram
actor App as Application
participant Engine as zr_engine_t
participant FBNext as fb_next
participant FBStage as fb_stage (snapshot)
participant FBPrev as fb_prev
participant Diff as diff_logic
App->>Engine: submit_drawlist(cmds)
alt fb_next not synced to fb_prev
Engine->>FBStage: capture snapshot of fb_next
Note over FBStage: rollback source ready
else fb_next already synced
Note over FBStage: snapshot not needed
end
Engine->>FBNext: apply drawlist commands
alt success
Engine->>Engine: mark fb_next_synced_to_prev = false
Note over Engine: sync state cleared after submit
else failure/rollback
alt snapshot exists
Engine->>FBNext: restore from fb_stage
else no snapshot
Engine->>FBNext: restore from fb_prev
end
Engine->>Engine: rollback complete
end
App->>Engine: present(metrics)
Engine->>Diff: compare fb_prev vs fb_next
Diff->>Diff: check exact link equality
alt links match exactly
Diff->>Diff: use damage_rect_copy
else links differ
Diff->>Diff: verify link targets equality
Diff->>Diff: compute damage rects if targets differ
end
Engine->>FBPrev: clone links from presented_fb
Engine->>Engine: update fb_next_synced_to_prev
Engine->>App: return metrics with damage info
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/unicode/zr_wrap.h (1)
8-9: Missing C++ linkage guards.The PR objectives state "Adds C++ guards to public headers," but this header lacks
extern "C"wrappers. For consistency with other headers in this PR and C++ compatibility, consider adding guards.,
♻️ Proposed fix
`#ifndef` ZR_UNICODE_ZR_WRAP_H_INCLUDED `#define` ZR_UNICODE_ZR_WRAP_H_INCLUDED + +#ifdef __cplusplus +extern "C" { +#endif `#include` "unicode/zr_width.h"And at the end of the file:
+#ifdef __cplusplus +} +#endif + `#endif` /* ZR_UNICODE_ZR_WRAP_H_INCLUDED */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/unicode/zr_wrap.h` around lines 8 - 9, This header is missing C++ linkage guards; wrap the public declarations with extern "C" when compiling under C++ by adding an `#ifdef` __cplusplus / extern "C" { ... } / `#endif` pair around the content of the file (i.e., immediately after the existing include guard macro ZR_UNICODE_ZR_WRAP_H_INCLUDED is defined and before the final `#endif`), so all symbols in this header use C linkage when included from C++.tests/unit/test_engine_submit_drawlist_no_partial_effects.c (1)
159-177: Extract new test literals into named constants and add AAA section markers.This path is readable today, but repeated hard-coded sizes/dimensions and no test sections make future updates brittle.
♻️ Suggested cleanup
+enum { + ZR_TEST_DL_HEADER_SIZE = 64u, + ZR_TEST_CMD_HEADER_SIZE = 8u, + ZR_TEST_CMD_BLIT_RECT_SIZE = 32u, + ZR_TEST_FB_COLS = 10u, + ZR_TEST_FB_ROWS = 4u, +}; static size_t zr_make_dl_invalid_blit_rect(uint8_t* out, size_t out_cap) { - const uint32_t cmd_bytes = 8u + 32u; - const uint32_t total_size = 64u + cmd_bytes; + const uint32_t cmd_bytes = ZR_TEST_CMD_HEADER_SIZE + ZR_TEST_CMD_BLIT_RECT_SIZE; + const uint32_t total_size = ZR_TEST_DL_HEADER_SIZE + cmd_bytes; size_t at = 0u; ... } ZR_TEST_UNIT(engine_submit_drawlist_invalid_blit_rect_has_no_partial_effects) { + /* --- Arrange --- */ ... - mock_plat_set_size(10u, 4u); + mock_plat_set_size(ZR_TEST_FB_COLS, ZR_TEST_FB_ROWS); ... - mock_plat_set_size(10u, 4u); + mock_plat_set_size(ZR_TEST_FB_COLS, ZR_TEST_FB_ROWS); + /* --- Act --- */ ... + /* --- Assert --- */ ZR_ASSERT_TRUE(a_len == b_len); ZR_ASSERT_MEMEQ(a_bytes, b_bytes, a_len); }As per coding guidelines "Extract magic numbers to named constants with descriptive names instead of using bare literals" and "Add section markers in long functions ... and in tests (
/* --- Arrange/Act/Assert --- */)."Also applies to: 284-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_engine_submit_drawlist_no_partial_effects.c` around lines 159 - 177, The test helper zr_make_dl_invalid_blit_rect contains multiple magic literals (e.g., 8u, 32u, 64u, 9u, framebuffer dims like 10x4 and w=2) and lacks Arrange/Act/Assert section markers; refactor by introducing descriptive named constants (e.g., CMD_HEADER_BYTES, BLIT_PAYLOAD_BYTES, DL_TOTAL_SIZE, INVALID_SRC_X, FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT, BLIT_W, BLIT_H) and replace the bare literals in zr_make_dl_invalid_blit_rect with those constants, then add AAA comments (/* --- Arrange --- */, /* --- Act --- */, /* --- Assert --- */) to separate logical sections; apply the same cleanup pattern to the other test block referenced around lines 284-317.tests/unit/test_diff_hotpath_telemetry.c (2)
16-26: Consider centralizingzr_bytes_containsin shared test helpers.
zr_bytes_containshere is effectively the same implementation astests/unit/test_diff_extended_styles.c(Line 112-122). Pulling it into shared test utility code would reduce duplication drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_diff_hotpath_telemetry.c` around lines 16 - 26, zr_bytes_contains is duplicated across tests; move it into a single shared test helper and update tests to use it: extract the function (remove static) into a common test helper header/source (e.g., tests/helpers) so both test_diff_hotpath_telemetry.c and test_diff_extended_styles.c include the helper, delete the duplicate implementations in each test, and ensure the helper is declared with external linkage or inline in the header so both tests link properly.
296-361: Split this test into helpers to satisfy function-length limits.The new test is ~66 lines and exceeds the project cap for C functions. Extract setup/scratch initialization into helper(s) to keep this unit under 50 lines.
Refactor sketch
+static void zr_init_diff_scratch_10(zr_diff_scratch_t* scratch, uint64_t* prev_hashes, + uint64_t* next_hashes, uint8_t* dirty_rows) { + memset(scratch, 0, sizeof(*scratch)); + scratch->prev_row_hashes = prev_hashes; + scratch->next_row_hashes = next_hashes; + scratch->dirty_rows = dirty_rows; + scratch->row_cap = 10u; + scratch->prev_hashes_valid = 0u; +} + ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs) { - /* full setup + scratch wiring inline */ + /* arrange */ + /* act */ + /* assert */ }As per coding guidelines,
**/*.c: "Functions should be 20-40 lines, with a maximum of 50 lines".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_diff_hotpath_telemetry.c` around lines 296 - 361, The test function ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs) is too long; extract repeated/setup code into small helpers: create one helper (e.g., setup_fb_with_link) that initializes zr_fb_t prev/next, clears them, interns the URI (wrapping zr_fb_init, zr_fb_clear, zr_fb_link_intern) and returns framebuffer(s) and link refs, and another helper (e.g., init_diff_scratch_and_caps) that initializes zr_limits_t lim, damage array, prev_hashes/next_hashes/dirty_rows, fills zr_diff_scratch_t fields (prev_row_hashes/next_row_hashes/dirty_rows/row_cap/prev_hashes_valid), platform caps (zr_caps_default + supports_hyperlinks), and zr_term_state_t initial; leave only the core assertions and the zr_diff_render_ex call in the test body so the test function length falls under 50 lines and naming (setup_fb_with_link, init_diff_scratch_and_caps, and ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs)) make locating changes trivial.tests/unit/test_engine_present_damage_resync_hyperlinks.c (1)
61-63: Return typeuint8_tfor boolean helper is unconventional.
zr_builder_reservereturnsuint8_t(0 or 1) for a boolean check. Consider usingboolfor clarity, or add a brief comment noting the convention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_engine_present_damage_resync_hyperlinks.c` around lines 61 - 63, The helper zr_builder_reserve currently returns a uint8_t for a boolean check; change its return type to bool for clarity by updating the function signature of zr_builder_reserve(const zr_dl_builder_t* b, size_t bytes) to return bool, add the appropriate include (stdbool.h) if missing, and ensure any callers or tests expecting uint8_t are adapted to accept/compare a bool; alternatively, if you must keep uint8_t, add a one-line comment above zr_builder_reserve documenting that it returns 0/1 as a boolean.src/core/zr_engine_present.inc (1)
337-341: Consider using checked arithmetic for thelinks_bytescalculation.While
links_lenis limited to 65535 by the framebuffer API, the local multiplication(size_t)prefix->links_len * sizeof(zr_fb_link_t)lacks defensive bounds checking. This pattern is used elsewhere in the codebase withzr_checked_mul_size()(e.g.,zr_framebuffer.c:143). For consistency and defensive programming, apply the same approach:Suggested check
if (prefix->links_len != 0u) { - const size_t links_bytes = (size_t)prefix->links_len * sizeof(zr_fb_link_t); + size_t links_bytes = 0u; + if (!zr_checked_mul_size((size_t)prefix->links_len, sizeof(zr_fb_link_t), &links_bytes)) { + return false; + } if (memcmp(prefix->links, full->links, links_bytes) != 0) {Note: The same unprotected pattern exists in
zr_diff.c:331.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_engine_present.inc` around lines 337 - 341, The multiplication for links_bytes should be done with checked arithmetic: use zr_checked_mul_size(prefix->links_len, sizeof(zr_fb_link_t), &links_bytes) (referencing prefix->links_len and zr_fb_link_t) and if the helper returns false handle the overflow path (e.g., return false) before calling memcmp on prefix->links and full->links; mirror the pattern used elsewhere (e.g., zr_framebuffer.c) so the function in src/core/zr_engine_present.inc defends against size overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/zr_diff.c`:
- Around line 330-333: The memcmp length is computed with unchecked
multiplication (links_bytes = (size_t)a->links_len * sizeof(zr_fb_link_t)) which
may overflow; before multiplying, check that a->links_len <= SIZE_MAX /
sizeof(zr_fb_link_t) (or equivalent) and handle the overflow case (e.g., return
false or propagate an error) so you never call memcmp with a wrapped length;
update the check around a->links_len, compute links_bytes only after the guard,
and keep references to a->links_len, links_bytes, zr_fb_link_t and the memcmp
call consistent.
In `@src/core/zr_drawlist.c`:
- Around line 1633-1645: Change the error returns for BLIT payload/bounds checks
from ZR_ERR_INVALID_ARGUMENT to ZR_ERR_FORMAT: whenever the code validates
cmd->w/cmd->h/cmd->src_x/ src_y/dst_x/dst_y or the zr_checked_add_u32 overflow
checks (variables src_x_end, src_y_end, dst_x_end, dst_y_end) fail, and when the
end coordinates exceed fb->cols or fb->rows, return ZR_ERR_FORMAT instead of
ZR_ERR_INVALID_ARGUMENT so drawlist format violations are reported consistently.
In `@tests/unit/test_drawlist_blit_rect.c`:
- Line 413: The test contains a formatting violation around the assertion call
zr_assert_cell_ascii in tests/unit/test_drawlist_blit_rect.c; run the project's
clang-format pass (bash scripts/clang_format_check.sh --all) and fix the
formatting for the assertion and surrounding lines so the file conforms to the
repository style before merging.
---
Nitpick comments:
In `@src/core/zr_engine_present.inc`:
- Around line 337-341: The multiplication for links_bytes should be done with
checked arithmetic: use zr_checked_mul_size(prefix->links_len,
sizeof(zr_fb_link_t), &links_bytes) (referencing prefix->links_len and
zr_fb_link_t) and if the helper returns false handle the overflow path (e.g.,
return false) before calling memcmp on prefix->links and full->links; mirror the
pattern used elsewhere (e.g., zr_framebuffer.c) so the function in
src/core/zr_engine_present.inc defends against size overflow.
In `@src/unicode/zr_wrap.h`:
- Around line 8-9: This header is missing C++ linkage guards; wrap the public
declarations with extern "C" when compiling under C++ by adding an `#ifdef`
__cplusplus / extern "C" { ... } / `#endif` pair around the content of the file
(i.e., immediately after the existing include guard macro
ZR_UNICODE_ZR_WRAP_H_INCLUDED is defined and before the final `#endif`), so all
symbols in this header use C linkage when included from C++.
In `@tests/unit/test_diff_hotpath_telemetry.c`:
- Around line 16-26: zr_bytes_contains is duplicated across tests; move it into
a single shared test helper and update tests to use it: extract the function
(remove static) into a common test helper header/source (e.g., tests/helpers) so
both test_diff_hotpath_telemetry.c and test_diff_extended_styles.c include the
helper, delete the duplicate implementations in each test, and ensure the helper
is declared with external linkage or inline in the header so both tests link
properly.
- Around line 296-361: The test function
ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs)
is too long; extract repeated/setup code into small helpers: create one helper
(e.g., setup_fb_with_link) that initializes zr_fb_t prev/next, clears them,
interns the URI (wrapping zr_fb_init, zr_fb_clear, zr_fb_link_intern) and
returns framebuffer(s) and link refs, and another helper (e.g.,
init_diff_scratch_and_caps) that initializes zr_limits_t lim, damage array,
prev_hashes/next_hashes/dirty_rows, fills zr_diff_scratch_t fields
(prev_row_hashes/next_row_hashes/dirty_rows/row_cap/prev_hashes_valid), platform
caps (zr_caps_default + supports_hyperlinks), and zr_term_state_t initial; leave
only the core assertions and the zr_diff_render_ex call in the test body so the
test function length falls under 50 lines and naming (setup_fb_with_link,
init_diff_scratch_and_caps, and
ZR_TEST_UNIT(diff_row_cache_marks_dirty_when_hyperlink_targets_change_with_same_refs))
make locating changes trivial.
In `@tests/unit/test_engine_present_damage_resync_hyperlinks.c`:
- Around line 61-63: The helper zr_builder_reserve currently returns a uint8_t
for a boolean check; change its return type to bool for clarity by updating the
function signature of zr_builder_reserve(const zr_dl_builder_t* b, size_t bytes)
to return bool, add the appropriate include (stdbool.h) if missing, and ensure
any callers or tests expecting uint8_t are adapted to accept/compare a bool;
alternatively, if you must keep uint8_t, add a one-line comment above
zr_builder_reserve documenting that it returns 0/1 as a boolean.
In `@tests/unit/test_engine_submit_drawlist_no_partial_effects.c`:
- Around line 159-177: The test helper zr_make_dl_invalid_blit_rect contains
multiple magic literals (e.g., 8u, 32u, 64u, 9u, framebuffer dims like 10x4 and
w=2) and lacks Arrange/Act/Assert section markers; refactor by introducing
descriptive named constants (e.g., CMD_HEADER_BYTES, BLIT_PAYLOAD_BYTES,
DL_TOTAL_SIZE, INVALID_SRC_X, FRAMEBUFFER_WIDTH, FRAMEBUFFER_HEIGHT, BLIT_W,
BLIT_H) and replace the bare literals in zr_make_dl_invalid_blit_rect with those
constants, then add AAA comments (/* --- Arrange --- */, /* --- Act --- */, /*
--- Assert --- */) to separate logical sections; apply the same cleanup pattern
to the other test block referenced around lines 284-317.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (66)
docs/abi/c-abi-reference.mddocs/abi/drawlist-format.mddocs/user-guide/rendering-model.mdexamples/zr_example_common.hinclude/zr/zr_caps.hinclude/zr/zr_config.hinclude/zr/zr_debug.hinclude/zr/zr_drawlist.hinclude/zr/zr_engine.hinclude/zr/zr_event.hinclude/zr/zr_metrics.hinclude/zr/zr_platform_types.hinclude/zr/zr_result.hinclude/zr/zr_terminal_caps.hinclude/zr/zr_version.hsrc/core/zr_cursor.hsrc/core/zr_damage.csrc/core/zr_damage.hsrc/core/zr_diff.csrc/core/zr_drawlist.csrc/core/zr_engine.csrc/core/zr_engine_present.incsrc/core/zr_event_pack.csrc/core/zr_event_pack.hsrc/core/zr_framebuffer.csrc/core/zr_framebuffer.hsrc/core/zr_metrics.csrc/core/zr_placeholder.csrc/platform/win32/zr_win32_conpty_test.csrc/platform/win32/zr_win32_conpty_test.hsrc/unicode/zr_grapheme.hsrc/unicode/zr_unicode_data.hsrc/unicode/zr_unicode_pins.hsrc/unicode/zr_utf8.hsrc/unicode/zr_width.hsrc/unicode/zr_wrap.hsrc/util/zr_arena.hsrc/util/zr_ring.hsrc/util/zr_string_builder.hsrc/util/zr_string_view.hsrc/util/zr_vec.htests/CMakeLists.txttests/golden/zr_golden.ctests/golden/zr_golden.htests/integration/win32_conpty_rawmode.ctests/integration/win32_conpty_wake.ctests/integration/zr_integration_smoke.ctests/unit/test_arena.ctests/unit/test_blit.ctests/unit/test_checked.ctests/unit/test_damage_rects.ctests/unit/test_diff_hotpath_telemetry.ctests/unit/test_drawlist_blit_rect.ctests/unit/test_drawlist_canvas.ctests/unit/test_drawlist_execute.ctests/unit/test_drawlist_image.ctests/unit/test_drawlist_validate.ctests/unit/test_engine_present_damage_resync_hyperlinks.ctests/unit/test_engine_present_single_flush.ctests/unit/test_engine_submit_drawlist_no_partial_effects.ctests/unit/test_engine_tick_events.ctests/unit/test_framebuffer_init_resize.ctests/unit/test_limits.ctests/unit/test_metrics_struct.ctests/zr_test.ctests/zr_tests_main.c
💤 Files with no reviewable changes (9)
- src/unicode/zr_utf8.h
- tests/unit/test_damage_rects.c
- tests/unit/test_arena.c
- tests/integration/zr_integration_smoke.c
- tests/zr_tests_main.c
- src/unicode/zr_unicode_pins.h
- tests/unit/test_checked.c
- src/util/zr_string_view.h
- tests/golden/zr_golden.h
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 333af95b07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/core/zr_framebuffer.c (2)
546-550: Heap allocation in compaction is justified but worth documenting.The
mallocfor the remap table could concern hot-path performance, but compaction is gated by growth/limit conditions (line 672), making it amortized rather than per-frame. Consider adding a brief comment noting this is intentional and amortized.📝 Suggested comment
+ /* + * Heap allocation is acceptable here because compaction only triggers when + * growth would occur or limits are exceeded—amortized, not per-frame. + */ uint32_t* remap = (uint32_t*)malloc(remap_bytes);Based on learnings: "Hot paths must avoid per-frame heap churn; buffer output and perform single flush per present operation"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_framebuffer.c` around lines 546 - 550, Add a brief comment above the remap allocation in src/core/zr_framebuffer.c explaining that the malloc for "remap" is intentional and not a per-frame hot-path allocation because it only occurs during compaction (refer to the remap variable and the compaction/growth/limit gating in this module), and that compaction is amortized by being triggered only on growth/limit conditions; keep the comment short and mention that this avoids per-frame heap churn while allowing a single flush during compaction.
507-625: Function exceeds 50-line limit; consider adding section markers.At ~112 lines, this function is well above the coding guideline maximum of 50 lines. The logic is linear and correct, but section markers would improve readability.
♻️ Proposed section markers
static zr_result_t zr_fb_links_compact_live(zr_fb_t* fb) { if (!fb) { return ZR_ERR_INVALID_ARGUMENT; } + /* --- Validate --- */ if (fb->links_len == 0u) { fb->link_bytes_len = 0u; return ZR_OK; } // ... validation continues ... + /* --- Allocate remap table --- */ size_t remap_count = 0u; // ... allocation ... - /* Mark live refs and sanitize any out-of-range link refs in cells. */ + /* --- Mark live refs --- */ for (size_t i = 0u; i < cell_count; i++) { // ... + /* --- Compact live entries --- */ const uint32_t old_links_len = fb->links_len; // ... + /* --- Update cell refs --- */ for (size_t i = 0u; i < cell_count; i++) { // ...Alternatively, the compaction loop (lines 569-607) could be extracted to a helper function like
zr_fb_links_compact_entries(fb, remap, &new_links_len, &new_link_bytes_len)to reduce cognitive load.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_framebuffer.c` around lines 507 - 625, The function zr_fb_links_compact_live exceeds the 50-line guideline—split it into clear sections or extract the inner compaction loop into a helper to reduce size and improve readability; either add section comments (e.g., "validate input", "mark live refs", "compact entries", "remap cells", "finalize") inside zr_fb_links_compact_live to break it up, or move the loop that builds new links/bytes (the body that iterates old_links_len, copies span bytes, updates new_links_len/new_link_bytes_len and sets remap) into a new helper like zr_fb_links_compact_entries(zr_fb_t* fb, uint32_t* remap, uint32_t old_links_len, uint32_t* out_new_links_len, uint32_t* out_new_link_bytes_len) and call it from zr_fb_links_compact_live while preserving error handling and remap semantics.tests/unit/test_limits.c (1)
215-268: Split this test into helpers and add test-phase section markers.This test is over the 50-line cap and lacks
/* --- Arrange/Act/Assert --- */sections, which makes it harder to maintain.♻️ Suggested refactor shape
+static void zr_limits_add_ephemeral_links(zr_fb_t* fb, zr_cell_t* left, zr_cell_t* right, + uint32_t* peak_links_len, uint32_t* peak_link_bytes_len) { + /* loop body extracted from test */ +} + ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) { + /* --- Arrange --- */ zr_fb_t fb; memset(&fb, 0, sizeof(fb)); ZR_ASSERT_EQ_U32(zr_fb_init(&fb, 2u, 1u), ZR_OK); ... - for (uint32_t i = 0u; i < 64u; i++) { - ... - } + /* --- Act --- */ + zr_limits_add_ephemeral_links(&fb, left, right, &peak_links_len, &peak_link_bytes_len); + /* --- Assert --- */ ZR_ASSERT_TRUE(peak_links_len <= ...); ... }As per coding guidelines,
**/*.c:Functions should be 20-40 lines, with a maximum of 50 linesandAdd section markers in long functions ... and in tests (/* --- Arrange/Act/Assert --- */, /* --- Validate/Compute/Emit --- */).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_limits.c` around lines 215 - 268, Split the long test function limits_link_intern_compacts_stale_refs_and_bounds_growth into smaller helpers and add explicit test-phase markers: extract setup into a helper like init_fb_with_persistent_link (handles zr_fb_init, persistent URI intern and assigning left/right cells), extract the loop that interns ephemeral URIs into a helper like intern_ephemeral_links_and_track_peaks (returns peak_links_len and peak_link_bytes_len), and extract the final lookup/assertions into a validate_persistent_link_lookup helper; then replace the body of ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) with three short sections marked with comments /* --- Arrange --- */, /* --- Act --- */, and /* --- Assert --- */ calling those helpers so the top-level test stays under 50 lines and follows the Arrange/Act/Assert structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_limits.c`:
- Around line 235-253: Replace the magic literals in the test loop with
descriptive named constants: introduce e.g. URI_BUF_SIZE (replace 96 used in
char uri_buf[96] and sizeof in snprintf), MAX_EPHEMERAL_URIS (replace 64 loop
bound), and EXPECTED_MAX_PEAK_LINKS (replace 5 used in assertions), then use
these constants in the snprintf call, the for-loop condition, and the final
ZR_ASSERT_TRUE checks (including the peak_link_bytes_len expression that
currently multiplies 5 by (ZR_FB_LINK_URI_MAX_BYTES + ZR_FB_LINK_ID_MAX_BYTES));
update references to uri_buf, i, peak_links_len and peak_link_bytes_len
accordingly so the test documents intent and avoids hardcoded thresholds.
- Around line 228-229: Replace explicit NULL comparisons with the
project-standard `!ptr` style for pointer checks: update assertions like
`ZR_ASSERT_TRUE(left != NULL)` and `ZR_ASSERT_TRUE(right != NULL)` (and any
occurrences for `out_uri`, `out_id`) to use `ZR_ASSERT_TRUE(!left) /
ZR_ASSERT_TRUE(!right)` style equivalently (or the inverse
`ZR_ASSERT_FALSE(!ptr)` depending on existing macros) so all pointer checks in
the test file use the `!ptr` idiom; search for `left`, `right`, `out_uri`, and
`out_id` and update their `== NULL`/`!= NULL` comparisons to the `!ptr` form
while preserving the original assertion logic and macro usage (e.g.,
`ZR_ASSERT_TRUE`).
---
Nitpick comments:
In `@src/core/zr_framebuffer.c`:
- Around line 546-550: Add a brief comment above the remap allocation in
src/core/zr_framebuffer.c explaining that the malloc for "remap" is intentional
and not a per-frame hot-path allocation because it only occurs during compaction
(refer to the remap variable and the compaction/growth/limit gating in this
module), and that compaction is amortized by being triggered only on
growth/limit conditions; keep the comment short and mention that this avoids
per-frame heap churn while allowing a single flush during compaction.
- Around line 507-625: The function zr_fb_links_compact_live exceeds the 50-line
guideline—split it into clear sections or extract the inner compaction loop into
a helper to reduce size and improve readability; either add section comments
(e.g., "validate input", "mark live refs", "compact entries", "remap cells",
"finalize") inside zr_fb_links_compact_live to break it up, or move the loop
that builds new links/bytes (the body that iterates old_links_len, copies span
bytes, updates new_links_len/new_link_bytes_len and sets remap) into a new
helper like zr_fb_links_compact_entries(zr_fb_t* fb, uint32_t* remap, uint32_t
old_links_len, uint32_t* out_new_links_len, uint32_t* out_new_link_bytes_len)
and call it from zr_fb_links_compact_live while preserving error handling and
remap semantics.
In `@tests/unit/test_limits.c`:
- Around line 215-268: Split the long test function
limits_link_intern_compacts_stale_refs_and_bounds_growth into smaller helpers
and add explicit test-phase markers: extract setup into a helper like
init_fb_with_persistent_link (handles zr_fb_init, persistent URI intern and
assigning left/right cells), extract the loop that interns ephemeral URIs into a
helper like intern_ephemeral_links_and_track_peaks (returns peak_links_len and
peak_link_bytes_len), and extract the final lookup/assertions into a
validate_persistent_link_lookup helper; then replace the body of
ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) with
three short sections marked with comments /* --- Arrange --- */, /* --- Act ---
*/, and /* --- Assert --- */ calling those helpers so the top-level test stays
under 50 lines and follows the Arrange/Act/Assert structure.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/zr_diff.c (1)
1804-1821:⚠️ Potential issue | 🟡 MinorUpdate remaining intrusive-link terminology for consistency.
The helper now uses
rect._link, but Line 1947 still saysrect.y0, which is stale and can mislead future maintenance.Suggested doc-only fix
- /* Active list links are stored intrusively in rect.y0 via zr_diff_rect_link_*(). */ + /* Active list links are stored intrusively in rect._link via zr_diff_rect_link_*(). */As per coding guidelines, "Comments in complex paths must explain decision rationale and tradeoffs, not just field glossaries."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/zr_diff.c` around lines 1804 - 1821, Update the stale doc-comment that still references rect.y0 to reflect the new intrusive-link field usage; specifically update the explanatory comment above zr_diff_rect_link_get/zr_diff_rect_link_set (and nearby coalescing comment) to mention that rect._link is used as a temporary intrusive "next" index during indexed coalescing and remove or replace the obsolete rect.y0 reference so the rationale and tradeoffs accurately describe the use of _link in those functions.
🧹 Nitpick comments (2)
tests/unit/test_drawlist_blit_rect.c (1)
71-95: Extract wire-format literals into named constants.The helper currently embeds protocol/layout constants (
64u,60u,32u, header field zeros, etc.). Promote these to local named constants/enums to document intent and reduce brittle maintenance.As per coding guidelines,
Extract magic numbers to named constants with descriptive namesandReplace magic thresholds and protocol constants with named constants in local enums or macros.Also applies to: 99-105, 124-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_drawlist_blit_rect.c` around lines 71 - 95, In zr_builder_finish, replace the hard-coded wire-format literals (e.g. 64u header offset, magic values like 0x4C44525Au is fine but numeric fields such as 64u, cmd_bytes offsets, the many trailing 0u fields, and any other magic thresholds found later at lines 99-105 and 124-159) with named constants or a local enum (e.g. HEADER_SIZE, HEADER_FIELD_COUNT, CMD_SECTION_OFFSET, RESERVED_FIELD_ZERO) and use those constants when calling zr_w32 and computing total/cmd_bytes; update any related variables (total, cmd_bytes) to use the new names for clarity so the protocol layout intent is documented and maintenance is easier.tests/unit/test_limits.c (1)
215-267: Break this test into sections (and keep under the max function length).This test is now over the file guideline max and is harder to scan without Arrange/Act/Assert markers. Please split helper/setup logic or add explicit test sections.
As per coding guidelines,
Functions should be 20-40 lines, with a maximum of 50 linesandAdd section markers in long functions ... and in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_limits.c` around lines 215 - 267, The test ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) is too long—split it into smaller logical sections (Arrange/Act/Assert) or extract setup/teardown and repeated loops into helper functions to keep each function ≤50 lines; e.g., move fb initialization and persistent link creation into a helper (using zr_fb_init and zr_fb_link_intern), move the ephemeral-link loop into its own helper that takes zr_cell_t* left/right and updates peak_* values, and keep final assertions and the zr_fb_link_lookup/zr_fb_release checks in the main test body so the top-level test reads as clear Arrange, Act, Assert sections. Ensure helper names are descriptive and referenced from the main test so reviewers can locate code by symbols like zr_fb_init, zr_fb_link_intern, zr_fb_cell, zr_fb_link_lookup, and zr_fb_release.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/zr_drawlist.c`:
- Around line 1027-1043: In zr_dl_validate_cmd_blit_rect, add checks to reject
negative coordinates in the parsed zr_dl_cmd_blit_rect_t (specifically
cmd.src_x, cmd.src_y, cmd.dst_x, cmd.dst_y) in addition to the existing
width/height checks; if any of these four fields are < 0 return ZR_ERR_FORMAT so
malformed blit command payloads are rejected early during
zr_dl_validate_cmd_blit_rect rather than deferred to later stages.
In `@tests/unit/test_drawlist_blit_rect.c`:
- Around line 189-193: Replace explicit NULL comparisons with pointer-truthiness
style: in zr_assert_cell_ascii (use the local pointer c returned from
zr_fb_cell_const) change the assertion that currently reads ZR_ASSERT_TRUE(c !=
NULL) to use the project `!ptr` style (e.g. assert non-null by inverting the
truthiness: ZR_ASSERT_FALSE(!c)); do the same for the other occurrence around
the c pointer at the later reported location (line ~367), ensuring all NULL
checks use `!ptr` style consistently.
---
Outside diff comments:
In `@src/core/zr_diff.c`:
- Around line 1804-1821: Update the stale doc-comment that still references
rect.y0 to reflect the new intrusive-link field usage; specifically update the
explanatory comment above zr_diff_rect_link_get/zr_diff_rect_link_set (and
nearby coalescing comment) to mention that rect._link is used as a temporary
intrusive "next" index during indexed coalescing and remove or replace the
obsolete rect.y0 reference so the rationale and tradeoffs accurately describe
the use of _link in those functions.
---
Nitpick comments:
In `@tests/unit/test_drawlist_blit_rect.c`:
- Around line 71-95: In zr_builder_finish, replace the hard-coded wire-format
literals (e.g. 64u header offset, magic values like 0x4C44525Au is fine but
numeric fields such as 64u, cmd_bytes offsets, the many trailing 0u fields, and
any other magic thresholds found later at lines 99-105 and 124-159) with named
constants or a local enum (e.g. HEADER_SIZE, HEADER_FIELD_COUNT,
CMD_SECTION_OFFSET, RESERVED_FIELD_ZERO) and use those constants when calling
zr_w32 and computing total/cmd_bytes; update any related variables (total,
cmd_bytes) to use the new names for clarity so the protocol layout intent is
documented and maintenance is easier.
In `@tests/unit/test_limits.c`:
- Around line 215-267: The test
ZR_TEST_UNIT(limits_link_intern_compacts_stale_refs_and_bounds_growth) is too
long—split it into smaller logical sections (Arrange/Act/Assert) or extract
setup/teardown and repeated loops into helper functions to keep each function
≤50 lines; e.g., move fb initialization and persistent link creation into a
helper (using zr_fb_init and zr_fb_link_intern), move the ephemeral-link loop
into its own helper that takes zr_cell_t* left/right and updates peak_* values,
and keep final assertions and the zr_fb_link_lookup/zr_fb_release checks in the
main test body so the top-level test reads as clear Arrange, Act, Assert
sections. Ensure helper names are descriptive and referenced from the main test
so reviewers can locate code by symbols like zr_fb_init, zr_fb_link_intern,
zr_fb_cell, zr_fb_link_lookup, and zr_fb_release.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/core/zr_diff.csrc/core/zr_drawlist.csrc/core/zr_engine_present.inctests/unit/test_drawlist_blit_rect.ctests/unit/test_limits.c
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_limits.c (1)
215-273: Refactor test into sections/smaller helpers to satisfy local C test style rules.Line 215 introduces a test body that is ~59 lines and currently lacks explicit
/* --- Arrange/Act/Assert --- */section markers. Please split the flow (or extract helpers) so the function stays within the repo’s size limit and uses section markers for readability.As per coding guidelines,
Functions should be 20-40 lines, with a maximum of 50 linesandAdd section markers in long functions (/* --- Section Name --- */) and in tests (/* --- Arrange/Act/Assert --- */, /* --- Validate/Compute/Emit --- */).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_limits.c` around lines 215 - 273, The test limits_link_intern_compacts_stale_refs_and_bounds_growth is too long and lacks section markers; split it into clearer Arrange/Act/Assert sections or extract small helper functions to keep each function ≤50 lines. Refactor by moving setup into a helper (e.g., setup_fb_and_persistent_link to init zr_fb_t, create persistent_uri and set left->style.link_ref), move the loop into a helper (e.g., create_ephemeral_links to intern ephemerals, update right->style.link_ref and track peak values), and move final lookups/assertions into a validate_results helper; add explicit comment markers /* --- Arrange --- */, /* --- Act --- */, /* --- Assert --- */ (or similar) inside the test and ensure the main test body only orchestrates calls to these helpers so the test function length is reduced and follows the style rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/test_limits.c`:
- Around line 215-273: The test
limits_link_intern_compacts_stale_refs_and_bounds_growth is too long and lacks
section markers; split it into clearer Arrange/Act/Assert sections or extract
small helper functions to keep each function ≤50 lines. Refactor by moving setup
into a helper (e.g., setup_fb_and_persistent_link to init zr_fb_t, create
persistent_uri and set left->style.link_ref), move the loop into a helper (e.g.,
create_ephemeral_links to intern ephemerals, update right->style.link_ref and
track peak values), and move final lookups/assertions into a validate_results
helper; add explicit comment markers /* --- Arrange --- */, /* --- Act --- */,
/* --- Assert --- */ (or similar) inside the test and ensure the main test body
only orchestrates calls to these helpers so the test function length is reduced
and follows the style rules.
…lidation-and-damage-link-sync # Conflicts: # docs/abi/c-abi-reference.md # docs/abi/drawlist-format.md # docs/user-guide/rendering-model.md # include/zr/zr_version.h # src/core/zr_drawlist.c # src/core/zr_engine_present.inc # tests/unit/test_drawlist_blit_rect.c # tests/unit/test_engine_submit_drawlist_no_partial_effects.c
RtlZeroMemory
deleted the
fix/present-hash-invalidation-and-damage-link-sync
branch
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
extern "C"guards to public headersValidation
ctest --test-dir build --output-on-failure(12/12)ctest --test-dir build-asan --output-on-failure(12/12)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests