Integrate SyntaxHighlighting to the editor - #89
Conversation
…ation and better organisation, lot of work arounds
WalkthroughAdds syntax definition files and build symlink, refactors the highlighting engine to use per-string end-stacks, exposes a tag lookup API, extends textlayout binding/update APIs with multi-line propagation, integrates highlighting into Editor and startup, and adds config accessors and small utility fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Main as main.c
participant Loader as SyntaxLoader
participant Editor
participant Binding as SH_Binding
participant Engine as Highlighter
User->>Main: start
Main->>Loader: Load syntax (Config_GetSyntax)
Loader-->>Main: SyntaxHighlighting* / error
Main->>Editor: create editor with highlighting
Editor->>Binding: Init binding (link highlighting)
User->>Editor: type / edit
Editor->>Binding: UpdateLine(current)
Binding->>Engine: Recalculate tags for line (use end-stack)
Engine-->>Binding: tags + open_blocks_at_end
alt end-stack changed
Binding->>Binding: update_following_lines()
loop per subsequent line
Binding->>Engine: Recalculate next line
Engine-->>Binding: new open_blocks_at_end
end
end
Editor->>Engine: GetTag(offset)
Engine-->>Editor: tag (color)
Editor->>User: render colored character
sequenceDiagram
autonumber
participant UpdateLine
participant Engine
participant Following
UpdateLine->>Engine: highlight current line (produce end-stack)
Engine-->>UpdateLine: open_blocks_at_end
alt end-stack differs from stored begin-stack
UpdateLine->>Following: update_following_lines()
loop until stacks match or EOF
Following->>Engine: highlight next line
Engine-->>Following: new end-stack
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/syntax/textlayoutbindings.c (1)
46-82: Gap merge inside recursion can be skipped; make sure current line is merged even when recursingIf prev_line isn’t highlighted, you recurse and return before reaching the current-line MergeGap block, so current line may be processed without merging the gap. Merge before recursing.
- Line *prev_line = line->prev; + const Line *prev_line = line->prev; @@ - else { + else { // highlighting for the line is not calculated so far - // so run this function for thr previous line + // so run this function for the previous line + if (line == binding->tl->tb->current_line) { + TextBuffer_MergeGap((TextBuffer*)binding->tl->tb); + } SyntaxHighlightingBinding_UpdateLine(binding, prev_line, last_line); return; } } - if (line == binding->tl->tb->current_line) { + if (line == binding->tl->tb->current_line) { // this is super dirty cause the gap funcitonality is totally disabled this way!!! // NEED A BETTER SOLUTION TextBuffer_MergeGap((TextBuffer*)binding->tl->tb); }Also consider making the prev-line walk iterative to avoid deep recursion on large files.
🧹 Nitpick comments (12)
src/common/table.c (1)
97-97: Remove unreachable code.This line is unreachable due to the guaranteed loop termination explained in lines 94-95. While it serves as documentation, it's dead code that could be removed.
Apply this diff:
- logFatal("That's not possible!");data/syntax/md.ini (4)
9-21: Inconsistent child_blocks declaration across title blocks.Line 13 explicitly sets
child_blocks =(empty), while title2 and title3 blocks (lines 15-21) omit this field entirely. For consistency and clarity, either includechild_blocks =in all blocks or omit it uniformly when there are no children.
23-25: Clarify the distinction betweencodeandcodeblockblocks.The
codeblock uses`.*`which matches inline code (backtick to end of line), whilecodeblockuses fenced code blocks with^````. Consider renamingcodetoinline_code` or adding a comment to clarify the distinction and prevent confusion.
32-34: Custom Markdown extension detected.The
cmdblock (lines starting with!) is not part of standard Markdown. If this is a custom extension for your editor, consider adding a comment explaining its purpose for maintainability.
36-41: Missing final newline in file.The file ends without a trailing newline after line 41. While not critical, adding a final newline is a best practice and many tools expect it.
src/syntax/highlighting.h (1)
76-86: Document that offset is a byte index into shs->text and consider an “active block” query.The contract says “offset in bytes,” but call sites can easily confuse char index vs byte index with UTF‑8. Please make this explicit (e.g., “offset is a byte index into shs->text->bytes”) and add a brief warning. Also consider a companion API that returns the active block at an arbitrary offset (not only exact tag boundaries), which would better fit per‑char rendering without probing each position for equality.
src/widgets/components/editor.c (2)
39-44: Duplicate TextSelection_Deinit call.TextSelection_Deinit(&editor->ts) is called twice. Remove one of them.
Apply this diff:
static void editor_destroy(Widget *self) { Editor *editor = AS_EDITOR(self); Timer_Stop(editor->cursor_timer); SyntaxHighlightingBinding_Deinit(&editor->sh_binding); - TextSelection_Deinit(&editor->ts); TextLayout_Deinit(&editor->tl); TextEdit_Deinit(&editor->te); TextSelection_Deinit(&editor->ts); }
440-443: Use the binding init helper instead of manual field assignment.Prefer the initializer for consistency and future changes; let Update run later once sh is assigned in main.
Apply this diff:
- self->sh_binding.sh = NULL; - self->sh_binding.tl = &self->tl; - SyntaxHighlightingBinding_Update(&self->sh_binding); + SyntaxHighlightingBinding_Init(&self->sh_binding, &self->tl, NULL);src/syntax/highlighting.c (1)
89-100: GetTag is O(n) per call; consider binary search or streaming.Editor calls this per character, making rendering O(n·m). Tags are appended in increasing byte_offset; you can switch to a binary search or, better, have the caller stream through tags once per line (advance an index when byte_offset matches).
Example drop‑in binary search:
-const SyntaxHighlightingTag *SyntaxHighlightingString_GetTag(const SyntaxHighlightingString *shs, size_t offset) { - // TODO: Need massive optimization - if (!shs || shs->tags_count == 0) { - return NULL; - } - for (int i=(int)shs->tags_count-1; i>=0; i--) { - if (shs->tags[i].byte_offset == offset) { - return &shs->tags[i]; - } - } - return NULL; -} +const SyntaxHighlightingTag *SyntaxHighlightingString_GetTag(const SyntaxHighlightingString *shs, size_t offset) { + if (!shs || shs->tags_count == 0) return NULL; + size_t lo = 0, hi = shs->tags_count; + while (lo < hi) { + size_t mid = lo + ((hi - lo) >> 1); + size_t off = shs->tags[mid].byte_offset; + if (off < offset) lo = mid + 1; + else hi = mid; + } + if (lo < shs->tags_count && shs->tags[lo].byte_offset == offset) { + return &shs->tags[lo]; + } + return NULL; +}src/syntax/textlayoutbindings.c (3)
13-23: Harden stack_equal against NULL (optional) and document ownershipCurrent callers pass non-NULL, but making it defensive avoids footguns; also clarify pointer equality intent.
static bool stack_equal(const Stack *a, const Stack *b) { + if (a == b) return true; + if (!a || !b) return false; if (a->size != b->size) { return false; } for (size_t i=0; i<a->size; i++) { if (a->items[i] != b->items[i]) { return false; } } return true; }
25-39: Const-correctness and minor cleanup in helpers
- update_following_lines’ open_blocks is read-only; make it const and drop the local alias.
- Remove commented Stack_Destroy lines; the stacks are owned by SyntaxHighlightingString—replace with a brief comment if needed.
-static void update_following_lines(SyntaxHighlightingBinding *binding, const Line *first_line, const Line *last_line, Stack *open_blocks) { - const Stack *open_blocks_begin = open_blocks; +static void update_following_lines(SyntaxHighlightingBinding *binding, const Line *first_line, const Line *last_line, const Stack *open_blocks_begin) { @@ - //Stack_Destroy(open_blocks_end); + // NOTE: open_blocks_end is owned by SyntaxHighlightingString; do not free. @@ - if (open_blocks) { - //Stack_Destroy(open_blocks); - } + // NOTE: open_blocks was not allocated here; no cleanup required.Also applies to: 77-81
84-96: Update() flow LGTM; optional: do gap merge once here to avoid per-line hacksAs an alternative to merging in UpdateLine, merge the gap once per Update() call to reduce side effects in the inner routine.
const TextBuffer *tb = binding->tl->tb; + TextBuffer_MergeGap((TextBuffer*)tb); // optional: centralize gap handlingAlso ensure no other subsystems rely on an unmerged gap during Update().
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CMakeLists.txt(1 hunks)data/syntax/ini.ini(1 hunks)data/syntax/md.ini(1 hunks)src/common/string.c(1 hunks)src/common/table.c(1 hunks)src/main.c(4 hunks)src/syntax/definition.c(3 hunks)src/syntax/highlighting.c(7 hunks)src/syntax/highlighting.h(1 hunks)src/syntax/textlayoutbindings.c(4 hunks)src/syntax/textlayoutbindings.h(1 hunks)src/widgets/components/editor.c(4 hunks)src/widgets/components/editor.h(2 hunks)tests/test_syntax_highlighting.c(3 hunks)tests/test_syntax_textlayoutbindings.c(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/main.c (3)
src/common/config.c (1)
Config_SetFilename(93-100)src/syntax/highlighting.c (1)
SyntaxHighlighting_Deinit(110-122)src/syntax/loader.c (2)
SyntaxHighlighting_LoadFromFile(20-76)SyntaxHighlightingLoaderError_Deinit(12-17)
src/syntax/definition.c (1)
src/common/string.c (4)
String_FromView(433-435)String_Trim(591-612)String_Length(301-303)String_Deinit(264-281)
src/syntax/textlayoutbindings.c (4)
src/common/table.c (1)
Table_Get(265-282)src/syntax/highlighting.c (1)
SyntaxHighlighting_HighlightString(207-320)src/document/textlayout.c (1)
TextLayout_GetVisualLine(389-401)src/document/textbuffer.c (1)
TextBuffer_GetLastLine(113-119)
src/syntax/textlayoutbindings.h (1)
src/syntax/textlayoutbindings.c (4)
SyntaxHighlightingBinding_Init(3-6)SyntaxHighlightingBinding_Deinit(8-11)SyntaxHighlightingBinding_UpdateLine(46-82)SyntaxHighlightingBinding_Update(84-96)
src/widgets/components/editor.c (8)
src/syntax/textlayoutbindings.c (2)
SyntaxHighlightingBinding_Deinit(8-11)SyntaxHighlightingBinding_Update(84-96)src/document/textselection.c (1)
TextSelection_Deinit(25-30)src/common/table.c (1)
Table_Get(265-282)src/display/canvas.c (1)
Canvas_MoveCursor(131-134)src/document/textlayout.c (2)
TextLayout_GetCursorLayoutInfo(416-473)VisualLine_GetChar(97-125)src/common/stack.c (1)
Stack_Peek(123-128)src/syntax/highlighting.c (1)
SyntaxHighlightingString_GetTag(89-100)src/common/utf8_helper.c (1)
utf8_to_codepoint(57-96)
src/syntax/highlighting.h (1)
src/syntax/highlighting.c (1)
SyntaxHighlightingString_GetTag(89-100)
src/syntax/highlighting.c (1)
src/common/stack.c (3)
Stack_Peek(123-128)Stack_Push(105-114)Stack_Pop(116-121)
tests/test_syntax_textlayoutbindings.c (1)
src/syntax/textlayoutbindings.c (1)
SyntaxHighlightingBinding_UpdateLine(46-82)
🔇 Additional comments (14)
src/common/table.c (2)
126-131: Critical bug fix: Preserves custom key functions during table growth.This change correctly maintains the original table's hash, comparison, copy, and free functions when reallocating. Without this fix, tables created with
Table_CreatePtr()orTable_CreateCustom()would break during resize by reverting to the default string-based key handling.
71-72: Clarify or resolve the "+1 hotfix" technical debt.The comment indicates this is a temporary workaround for an edge case bug. Consider investigating and properly fixing the underlying issue rather than relying on this hotfix.
Can you clarify what edge case this addresses? If feasible, resolve the root cause to eliminate this workaround.
data/syntax/ini.ini (2)
13-13: Verify intended comment matching behavior.The comment start pattern changed from
"^(;|#)"(line-start anchored) to"(;|#)"(matches anywhere). This allows mid-line comments likekey = value ; comment. Ensure this aligns with your INI parsing requirements, as standard INI format typically only supports full-line comments.
41-45: bare_string pattern is correct—no issue found.The
start = "."pattern in thebare_stringblock is intentionally designed as a catch-all fallback for unquoted values, positioned last in theassignmentblock'schild_blockslist after more specific patterns (numberandstring). Theends_on = commentdirective properly terminates the match at INI comment markers. This follows standard syntax parser design where specific patterns are tried first and a general fallback is applied last. No conflict or unintended behavior occurs.src/common/string.c (1)
538-539: LGTM! Critical buffer safety fix.The updated capacity check ensures space for the null terminator when appending. The previous logic could allocate exactly
new_byte_sizebytes, but line 544 writes'\0'at positionnew_byte_size, requiringnew_byte_size + 1capacity.tests/test_syntax_highlighting.c (2)
68-68: LGTM! Added defensive NULL check.The assertion ensures the syntax definition is successfully created before proceeding, improving test reliability and providing clearer failure diagnostics.
327-366: LGTM! Comprehensive Markdown syntax test.The new test case validates Markdown title block parsing with proper offset tracking. The test expects the title block to start at offset 0 and end at offset 7 for the input "# Title", which aligns with the Markdown syntax definition.
src/widgets/components/editor.h (1)
25-25: LGTM! Clean syntax highlighting integration.The addition of the
sh_bindingfield to the Editor struct properly integrates syntax highlighting state with the editor lifecycle. The include and field placement are appropriate.Also applies to: 54-55
CMakeLists.txt (1)
57-62: Verify cross-platform symlink compatibility.The symlink creation enables access to syntax definition files in the build directory. However, symlinks require elevated privileges on Windows (unless Developer Mode is enabled). Consider using
file(COPY ...)or checking for symlink support failures.Add error handling or platform-specific logic:
# Alternative: Copy instead of symlink for better Windows compatibility if(WIN32) file(COPY ${CMAKE_SOURCE_DIR}/data DESTINATION ${CMAKE_BINARY_DIR}) else() execute_process( COMMAND ${CMAKE_COMMAND} -E create_symlink ${CMAKE_SOURCE_DIR}/data ${CMAKE_BINARY_DIR}/data ) endif()tests/test_syntax_textlayoutbindings.c (1)
116-116: LGTM! Tests updated for API rename.The function calls correctly reflect the API rename from
SyntaxHighlightingBinding_UpdatetoSyntaxHighlightingBinding_UpdateLine. The test logic and arguments remain unchanged, ensuring continuity.Also applies to: 143-143
src/syntax/definition.c (1)
220-246: LGTM! Robust handling of empty block names is properly implemented.The function correctly skips empty names after trimming and manages output count appropriately. The single call site at line 274 in
block_name_list_str_to_blocks()properly passes theout_countparameter, and error handling resets it on failure. Since existing syntax definitions contain no trailing commas, the empty-name filtering won't break current configurations.src/syntax/textlayoutbindings.h (1)
8-10: API shape looks good; confirm constness expectations.Making tl non‑const aligns with UpdateLine’s behavior (may affect layout). The new Update(...) entry point is a nice convenience.
Please confirm no remaining call sites still rely on a const TextLayout* contract; if any exist, adjust them to pass a non‑const pointer or update wrappers accordingly.
Also applies to: 12-13, 15-17
src/syntax/highlighting.c (1)
110-114: NULL‑guard in Deinit is good defensive coding.src/syntax/textlayoutbindings.c (1)
3-6: Init signature change verified—all call sites updated correctlyThe grep confirmed that
SyntaxHighlightingBinding_Initis called only once (in tests/test_syntax_textlayoutbindings.c:65) with the correct non-const signature. Declaration, definition, and call site are all consistent. No issues.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/common/config.h (1)
24-25: Fix prototype mismatch for Config_Init.Header declares void Config_Init(); implementation is void Config_Init(const char *argv0). This will not compile cleanly. Align the header.
Apply:
-void Config_Init(); +void Config_Init(const char *argv0);src/widgets/components/editor.c (1)
39-44: Remove duplicate TextSelection_Deinit call.
TextSelection_Deinit(&editor->ts)is called at both line 40 and line 43, which is redundant. Based on the TextSelection_Deinit implementation (src/document/textselection.c:24-29), calling it twice is safe but wasteful.Apply this diff:
Timer_Stop(editor->cursor_timer); SyntaxHighlightingBinding_Deinit(&editor->sh_binding); - TextSelection_Deinit(&editor->ts); TextLayout_Deinit(&editor->tl); TextEdit_Deinit(&editor->te); TextSelection_Deinit(&editor->ts);
♻️ Duplicate comments (5)
src/main.c (2)
192-192: Deinit loader error object on success as well.Prevents latent leaks if fields were populated. This was noted previously.
if (!highlighting) { ... exit(1); } + // success path: tidy up the error struct too + SyntaxHighlightingLoaderError_Deinit(&error);
224-225: Kick an initial highlight after wiring the engine.Editor_Init ran with sh==NULL; compute first highlight now.
- editor->editor->sh_binding.sh = highlighting; + editor->editor->sh_binding.sh = highlighting; + SyntaxHighlightingBinding_Update(&editor->editor->sh_binding);src/widgets/components/editor.c (2)
84-86: Add NULL guard for sh before dereferencing.Line 85 dereferences
sh->stringswithout verifying thatshis non-NULL. The binding can havesh == NULL(as seen in Editor_Init line 459 where it's initialized to NULL). This was flagged in a previous review but only the key type was fixed.Apply this diff:
// setup syntax highlighting stuff SyntaxHighlighting *sh = editor->sh_binding.sh; - SyntaxHighlightingString *shs = Table_Get(sh->strings, &line->src->text); + SyntaxHighlightingString *shs = NULL; + if (sh) { + shs = Table_Get(sh->strings, &line->src->text); + }
108-124: Fix undefined behavior and NULL handling in byte offset calculation.Line 116 has multiple critical issues:
- Duplicate assignment:
byte_offset = byte_offset = ...should bebyte_offset = ...- Undefined behavior: Computing
ch - line->src->text.byteswhenchmay point into gap text (returned by VisualLine_GetChar) is undefined behavior and produces incorrect offsets. This was flagged in a previous review.- NULL dereference: Line 118 calls
SyntaxHighlightingString_GetTag(shs, byte_offset)butshscan be NULL.The comments at lines 110-112 and 115-116 acknowledge the gap problem but don't fix it.
Apply this diff:
for (int i=0; i<line->length; i++) { - // get the character to draw (might not be in line->src->text.bytes if it's in the gap in general) - // in the moment the gap is always merged when updateing the syntax highlighting, so it should - // work at the moment - // Pay attention in future changes! - const char *ch = VisualLine_GetChar(line, i); - - // This will not always work if is_gap_line and the gap is not merged!! - size_t byte_offset = byte_offset = ch - line->src->text.bytes; + // Draw uses the visual character (may be in gap), but highlighting offsets must + // be computed against the source line text. + const char *ch_draw = VisualLine_GetChar(line, i); + const char *ch_src = String_GetChar(&line->src->text, line->offset + i); + size_t byte_offset = (size_t)(ch_src - line->src->text.bytes); - const SyntaxHighlightingTag *tag = SyntaxHighlightingString_GetTag(shs, byte_offset); + const SyntaxHighlightingTag *tag = shs ? SyntaxHighlightingString_GetTag(shs, byte_offset) : NULL; if (tag) { canvas->current_style.fg = tag->block->color; line_style.fg = tag->block->color; } - uint32_t cp = utf8_to_codepoint(ch); + uint32_t cp = utf8_to_codepoint(ch_draw);src/syntax/textlayoutbindings.c (1)
26-45: Stale cache issue remains unaddressed.The break condition at lines 30-35 was previously flagged: when a line's text is modified but its
open_blocks_at_begindoesn't change, the cachedshsbecomes stale but won't be recomputed becausestack_equalreturns true. The previous review suggested either:
- Invalidate the cache entry (via
Table_Delete(binding->sh->strings, &modified_line->text)) before callingupdate_following_lines, or- Track a per-line version/hash and include it in the break condition.
Additionally, line 39 has a commented-out
Stack_Destroycall with no explanation of why it's disabled. If the stack ownership has changed, document this in a comment; otherwise, remove the dead code.Run this script to check if any cache invalidation exists when lines are edited:
#!/bin/bash # Search for Table_Delete or cache invalidation on text edits rg -n --type=c 'Table_Delete.*strings|SyntaxHighlightingString_Clear' -B3 -A3 # Check where line text is modified rg -n --type=c 'String_(Set|Append|Insert|Take).*line.*text' -B2 -A2 | head -50
🧹 Nitpick comments (6)
src/syntax/textlayoutbindings.h (1)
4-4: Ensure C99/C11 availability for <stdbool.h> or add a portability shim.Static analysis flagged stdbool.h not found. Make sure the build uses -std=c99+ or include a common header that defines bool on older toolchains.
src/main.c (1)
168-171: Provide a default syntax when none is set.Avoid loader error on NULL syntax by defaulting (e.g., “md”).
- highlighting = SyntaxHighlighting_LoadFromFile(Config_GetSyntax(), &error); + const char *syntax = Config_GetSyntax(); + highlighting = SyntaxHighlighting_LoadFromFile(syntax ? syntax : "md", &error);data/syntax/md.ini (2)
36-41: Tighten fenced code end pattern.Anchor end to the fence-only line to avoid premature closes.
[block:codeblock] # section: [section_name] -start = "^```" -end = "^```" +start = "^```.*$" +end = "^```$" color = 64 # cyan child_blocks =
1-4: Consider broader extensions.Many files use .markdown/.mdown. Optionally include them.
-file_extensions = md +file_extensions = md, markdown, mdown, mkdsrc/syntax/highlighting.c (1)
89-100: LGTM: GetTag implementation with performance note.The function correctly performs a reverse linear search and handles NULL inputs safely. The TODO comment appropriately flags the O(n) performance concern. Consider optimizing with binary search (tags appear to be ordered by byte_offset) or a spatial index structure if this becomes a bottleneck during rendering.
src/syntax/textlayoutbindings.c (1)
80-82: Remove or document commented-out code.Lines 39, 81 have commented-out
Stack_Destroycalls with no explanation. If stack ownership has changed (e.g., stacks are now owned bySyntaxHighlightingStringand destroyed automatically), document this. Otherwise, remove the dead code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
data/syntax/md.ini(1 hunks)src/common/config.c(4 hunks)src/common/config.h(1 hunks)src/main.c(4 hunks)src/syntax/highlighting.c(8 hunks)src/syntax/textlayoutbindings.c(3 hunks)src/syntax/textlayoutbindings.h(1 hunks)src/widgets/components/editor.c(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/syntax/highlighting.c (1)
src/common/stack.c (3)
Stack_Peek(123-128)Stack_Push(105-114)Stack_Pop(116-121)
src/common/config.h (1)
src/common/config.c (2)
Config_SetSyntax(111-114)Config_GetSyntax(116-118)
src/widgets/components/editor.c (7)
src/syntax/textlayoutbindings.c (4)
SyntaxHighlightingBinding_Deinit(9-12)SyntaxHighlightingBinding_Update(85-97)SyntaxHighlightingBinding_UpdateAll(99-110)SyntaxHighlightingBinding_Init(3-7)src/document/textselection.c (1)
TextSelection_Deinit(25-30)src/document/textlayout.c (3)
TextLayout_GetVisualLine(389-401)VisualLine_GetChar(97-125)TextLayout_GetCursorLayoutInfo(416-473)src/common/table.c (1)
Table_Get(265-282)src/common/stack.c (1)
Stack_Peek(123-128)src/syntax/highlighting.c (1)
SyntaxHighlightingString_GetTag(89-100)src/common/utf8_helper.c (1)
utf8_to_codepoint(57-96)
src/syntax/textlayoutbindings.c (4)
src/common/table.c (1)
Table_Get(265-282)src/syntax/highlighting.c (1)
SyntaxHighlighting_HighlightString(210-323)src/document/textlayout.c (1)
TextLayout_GetVisualLine(389-401)src/document/textbuffer.c (2)
TextBuffer_GetLastLine(113-119)TextBuffer_GetFirstLine(105-111)
src/main.c (3)
src/common/config.c (3)
Config_SetFilename(97-104)Config_SetSyntax(111-114)Config_GetSyntax(116-118)src/syntax/highlighting.c (1)
SyntaxHighlighting_Destroy(133-139)src/syntax/loader.c (2)
SyntaxHighlighting_LoadFromFile(20-76)SyntaxHighlightingLoaderError_Deinit(12-17)
src/syntax/textlayoutbindings.h (1)
src/syntax/textlayoutbindings.c (5)
SyntaxHighlightingBinding_Init(3-7)SyntaxHighlightingBinding_Deinit(9-12)SyntaxHighlightingBinding_UpdateLine(47-83)SyntaxHighlightingBinding_Update(85-97)SyntaxHighlightingBinding_UpdateAll(99-110)
🪛 Clang (14.0.6)
src/syntax/textlayoutbindings.h
[error] 4-4: 'stdbool.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (11)
src/common/config.h (1)
37-39: New syntax API looks fine; confirm NULL handling at call sites.GetSyntax may return NULL until set. Ensure callers (e.g., loader) provide a default.
src/syntax/textlayoutbindings.h (1)
9-12: API updates LGTM.Non-const TextLayout*, added need_full_update, and Update/UpdateAll entry points align with usage in .c.
Please confirm all dependents were updated to the renamed UpdateLine symbol.
Also applies to: 14-20
src/widgets/components/editor.c (4)
354-354: LGTM: Syntax highlighting update after input.Correctly triggers syntax highlighting refresh after successful text editing input.
392-394: LGTM: Lazy syntax highlighting initialization.The use of
SyntaxHighlightingBinding_UpdateAllwithforce=falseensures highlighting is initialized once on first update via the internalneed_full_updateflag.
459-459: LGTM: Binding initialization.Correctly initializes the syntax highlighting binding with
sh=NULL, which will be set later by external code (e.g., main.c). This confirms thatshcan be NULL and must be guarded before use in draw_visual_line.
96-99: Guard against NULL shs before dereferencing.Line 96 calls
Stack_Peek(&shs->open_blocks_at_begin)without checking ifshsis NULL. Sinceshsis obtained fromTable_Getat line 85, it can be NULL if the line hasn't been highlighted yet.Apply this diff:
// change the style if it's the current line - if (shs && Stack_Peek(&shs->open_blocks_at_begin)) { - SyntaxBlockDef *first_block = Stack_Peek(&shs->open_blocks_at_begin); + SyntaxBlockDef *first_block = NULL; + if (shs) { + first_block = Stack_Peek(&shs->open_blocks_at_begin); + } + if (first_block) { - canvas->current_style.fg = first_block->color; + canvas->current_style.fg = first_block->color; }Likely an incorrect or invalid review comment.
src/syntax/highlighting.c (2)
111-113: LGTM: Defensive NULL check.Adding a NULL guard in
SyntaxHighlighting_Deinitimproves robustness.
134-136: LGTM: Defensive NULL check.Adding a NULL guard in
SyntaxHighlighting_Destroyimproves robustness.src/syntax/textlayoutbindings.c (3)
3-7: LGTM: Binding initialization with full update flag.Correctly initializes the
need_full_updateflag to trigger a complete highlighting pass on first use.
85-97: LGTM: Update API for visible region.Correctly computes the range from the current cursor line to the last visible line (or buffer end) and triggers highlighting via
UpdateLine.
99-110: LGTM: UpdateAll API for full document highlighting.Correctly implements the lazy full-update pattern using the
need_full_updateflag, and processes the entire buffer range viaUpdateLine.
| config.syntax = NULL; | ||
| } |
There was a problem hiding this comment.
Avoid dangling pointer: take ownership of syntax string and free on deinit.
Config_SetSyntax stores the caller’s pointer without copying; if the caller provides a transient buffer, this can dangle. Copy it and release previous.
@@
- config.colors = NULL;
- config.syntax = NULL;
+ config.colors = NULL;
+ config.syntax = NULL;
@@
-void Config_SetSyntax(const char *type) {
- config.syntax = type;
- config.dirty = true;
-}
+void Config_SetSyntax(const char *type) {
+ if (config.syntax) {
+ free((char*)config.syntax);
+ }
+ config.syntax = type ? strdup(type) : NULL;
+ config.dirty = true;
+}
@@
- config.colors = NULL;
- config.syntax = NULL;
+ config.colors = NULL;
+ if (config.syntax) {
+ free((char*)config.syntax);
+ config.syntax = NULL;
+ }Also applies to: 111-118
🤖 Prompt for AI Agents
In src/common/config.c around lines 61-62 (and similarly at lines 111-118),
Config_SetSyntax currently stores the caller's pointer directly which can lead
to a dangling pointer; change the implementation to take ownership by
strdup/copying the incoming syntax string, free any existing config.syntax
before assigning the new copy, and ensure the deinit/cleanup path frees
config.syntax (and sets it to NULL) to avoid leaks and use-after-free.
| #ifndef TESTFILE | ||
| static void print_help(const char *program_name) { | ||
| fprintf(stderr, "Usage:\n %s <filename>\n", program_name); | ||
| exit(0); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Fix parse_arguments: -h availability, TESTFILE fallback, and positional handling.
- print_help is compiled out when TESTFILE is defined but still called in -h branch → link error.
- Filename is set before getopt, so options like -s may be misinterpreted as filename.
- Unconditional trailing print_help causes exit even with TESTFILE fallback.
-#ifndef TESTFILE
-static void print_help(const char *program_name) {
+static void print_help(const char *program_name) {
fprintf(stderr, "Usage:\n %s <filename>\n", program_name);
exit(0);
}
-#endif
@@
-static void parse_arguments(int argc, char *argv[]) {
- // For now, we only handle a single filename argument.
- if (argc >= 2) {
- Config_SetFilename(argv[1]);
- } else {
-#ifdef TESTFILE
- Config_SetFilename(TESTFILE);
-#else
- print_help(argv[0]);
- // No file provided, could set a default or leave it empty.
- Config_SetFilename(NULL);
-#endif
- }
-
- int opt;
- while ((opt = getopt(argc, argv, "hs:")) != -1) {
+static void parse_arguments(int argc, char *argv[]) {
+ int opt;
+ optind = 1;
+ while ((opt = getopt(argc, argv, "hs:")) != -1) {
switch (opt) {
case 'h':
print_help(argv[0]);
return;
case 's':
Config_SetSyntax(optarg);
break;
case '?':
fprintf(stderr, "Unknown option: -%c\n", optopt);
exit(1);
}
}
-
- // Verbleibende Argumente:
- for (int i = optind; i < argc; i++) {
- Config_SetFilename(argv[i]);
- return;
- }
-
- print_help(argv[0]);
+ // first positional argument (filename)
+ if (optind < argc) {
+ Config_SetFilename(argv[optind]);
+ return;
+ }
+#ifdef TESTFILE
+ Config_SetFilename(TESTFILE);
+#else
+ print_help(argv[0]);
+ Config_SetFilename(NULL);
+#endif
}Also applies to: 60-67, 69-92
🤖 Prompt for AI Agents
In src/main.c around lines 48-53 (also apply to 60-67 and 69-92): print_help is
conditionally compiled out under TESTFILE but still invoked from the -h branch
and unconditionally at the end, the filename is assigned before getopt which
misparses options and TESTFILE fallback isn't respected. Fix by (1) moving
filename/positional argument handling to after getopt loop so options like -s
are processed first; (2) change the -h branch to only call print_help when not
compiled with TESTFILE (wrap the call in #ifndef TESTFILE or replace with a
fallback help message when TESTFILE is defined); (3) remove the unconditional
trailing call to print_help and instead check for missing filename and use the
TESTFILE fallback if defined, otherwise print usage/exit; and (4) ensure getopt
return values and optind are used to detect and assign the positional filename
after option parsing.
| static bool stack_equal(const Stack *a, const Stack *b) { | ||
| if (a->size != b->size) { | ||
| return false; | ||
| } | ||
| for (size_t i=0; i<a->size; i++) { | ||
| if (a->items[i] != b->items[i]) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add NULL checks to prevent crashes.
stack_equal dereferences a and b without NULL checks. If either stack pointer is NULL, this will crash at lines 15, 18, or 19.
Apply this diff:
static bool stack_equal(const Stack *a, const Stack *b) {
+ if (!a || !b) {
+ return (a == b); // Both NULL is equal, one NULL is not equal
+ }
if (a->size != b->size) {
return false;
}🤖 Prompt for AI Agents
In src/syntax/textlayoutbindings.c around lines 14 to 24, stack_equal currently
dereferences a and b without NULL checks which can crash; modify it to first
handle NULLs (if a == b return true; if one is NULL return false) before
accessing fields, then compare sizes and items as before so you never
dereference a or b when NULL.
| // highlighting for the line is not calculated so far | ||
| // so run this function for thr previous line | ||
| SyntaxHighlightingBinding_Update(binding, prev_line, last_line); | ||
| SyntaxHighlightingBinding_UpdateLine(binding, prev_line, last_line); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Recursive backward propagation may cause stack overflow.
Line 68 recursively calls SyntaxHighlightingBinding_UpdateLine when the previous line hasn't been highlighted yet. For a document with many unhighlighted lines, this creates a deep call stack (one frame per line). Consider refactoring to iterate backward to find the first highlighted line, then update forward iteratively.
Example iterative approach:
// Find the first highlighted line or the document start
const Line *start_line = line;
while (start_line->prev) {
SyntaxHighlightingString *prev_shs = Table_Get(binding->sh->strings, &start_line->prev->text);
if (prev_shs) {
break; // Found a highlighted line
}
start_line = start_line->prev;
}
// Now update forward from start_line🤖 Prompt for AI Agents
In src/syntax/textlayoutbindings.c around lines 66-70, the code recursively
calls SyntaxHighlightingBinding_UpdateLine on the previous line which can
overflow the stack for many unhighlighted lines; replace this recursion with an
iterative two-phase approach: walk backward from the current line until you find
a line that already has highlighting or the document start, then iterate forward
from that found start (or the first line) up to last_line and call the existing
update logic non-recursively for each line; remove the single recursive call and
ensure the forward loop updates each line in order so propagation happens
without growing the call stack.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main.c (3)
52-84: Fix CLI parsing: misordered filename handling, -s only breaks, and missing TESTFILE fallback.Parse options first, then pick positional filename; use TESTFILE only as fallback. Current code sets filename before getopt and unconditionally prints help at the end.
static void parse_arguments(int argc, char *argv[]) { - // For now, we only handle a single filename argument. - if (argc >= 2) { - Config_SetFilename(argv[1]); - } else { - print_help(argv[0]); - // No file provided, could set a default or leave it empty. - Config_SetFilename(NULL); - } - - int opt; - while ((opt = getopt(argc, argv, "hs:")) != -1) { + int opt; + optind = 1; + while ((opt = getopt(argc, argv, "hs:")) != -1) { switch (opt) { case 'h': print_help(argv[0]); return; case 's': Config_SetSyntax(optarg); break; case '?': fprintf(stderr, "Unknown option: -%c\n", optopt); exit(1); } } - // Verbleibende Argumente: - for (int i = optind; i < argc; i++) { - Config_SetFilename(argv[i]); - return; - } - - print_help(argv[0]); + // first positional argument (filename) + if (optind < argc) { + Config_SetFilename(argv[optind]); + return; + } +#ifdef TESTFILE + Config_SetFilename(TESTFILE); +#else + print_help(argv[0]); + Config_SetFilename(NULL); +#endif }
161-185: Deinit loader error struct on success as well.Prevents latent leaks if fields are ever populated; safe to call after success.
highlighting = SyntaxHighlighting_LoadFromFile(Config_GetSyntax(), &error); if (!highlighting) { @@ exit(1); } + // Success: tidy up loader error struct as well + SyntaxHighlightingLoaderError_Deinit(&error);
217-218: Kick an initial highlight update after binding.Editor_Init ran with sh==NULL; trigger first highlight now for immediate effect.
- editor->editor->sh_binding.sh = highlighting; + editor->editor->sh_binding.sh = highlighting; + SyntaxHighlightingBinding_Update(&editor->editor->sh_binding);
🧹 Nitpick comments (2)
src/main.c (2)
44-45: Limit symbol visibility for the global highlighting handle.Make it file‑local and explicitly NULL‑initialize to avoid exporting an unnecessary symbol.
-SyntaxHighlighting *highlighting; +static SyntaxHighlighting *highlighting = NULL;
128-129: Minor: guard destroy and null the pointer.Safer on repeated calls and future refactors.
- SyntaxHighlighting_Destroy(highlighting); + if (highlighting) { + SyntaxHighlighting_Destroy(highlighting); + highlighting = NULL; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
assets/screenshot.pngis excluded by!**/*.png
📒 Files selected for processing (2)
data/syntax/md.ini(1 hunks)src/main.c(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- data/syntax/md.ini
🧰 Additional context used
🧬 Code graph analysis (1)
src/main.c (3)
src/common/config.c (3)
Config_SetSyntax(111-114)Config_SetFilename(97-104)Config_GetSyntax(116-118)src/syntax/highlighting.c (1)
SyntaxHighlighting_Destroy(133-139)src/syntax/loader.c (2)
SyntaxHighlighting_LoadFromFile(20-76)SyntaxHighlightingLoaderError_Deinit(12-17)
🔇 Additional comments (1)
src/main.c (1)
37-38: Include looks correct.Header is appropriate for loader usage. No issues.
* add String_Split() and String_Trim() with tests * add example syntax definition * Feat/Implement Syntax Definitions (#84) * add SyntaxBlockDef and SyntaxDefinition declarations * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * Add table iteration API and improve parsing robustness (#83) * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * WIP: implement definition loader * add Table_GetUsage() * allow ':' in INI file key names * fix location of tests * fix typo in Doxygen comment * add NULL guard to String_Take() * Add early-exit guards to table lookup functions (#80) * Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete() * add functiond to create SyntaxDefinition_FromTable() with tests. More tests needed * additional tests (still not enough) * fix: free correct regex in regex_end branch in SyntaxBlockDef_FromTable() * fix: wrong numbe of arguments in String_Format() call in SymtaxBlockDef_FromTable() * add explanation * fix correct recognition of root block * fix memory lealk * add NULL guard for strdup in init_definition() * fix last fix * add additional test * add documentation * Feat/Syntax Highlighting: Add module to generate Highlighting information (#85) * add Stack_Copy() and Stack_IsEmpty() * fix: return copy in Stack_Copy() * implement syntax highlighting * add Table_CreateCustom() and Table_CreatePtr() * fix minor typing bugs * add Stack_Create(), Stack_Destroy() * fix but in find_first_child to actually return the child with the first occurence * automatically add patterns to end only_start blocks and to start/end root * fix memory leak produced in last commit * fix tests * fix bug in SyntaxHighllighting_HighlightString(): check if child or block end comes first * add basic tests * use root block it open_blocks parameter is NULL in SyntaxHighlighting_HighlightString() * add random tests * handle src->capacity == 0 in Stack_Copy() * add NULL pointer guard before using the key_free_func() function pointer in the Table module * fix error message for end_regex compilation error in SyntaxBlockDef_FromTable() * correct inlcudes * fix typo * add missing includes * change examples in documentation * change "key == NULL" to "!key" for non pointer keys * do not shadow parameter match in find_first_block() * change first parameter name in SyntaxHighlighting_HighlightString() for consitency * Cache regex matches and add ends_on option for Syntax definitions (#86) * add const to parameter if function does not modify it. add Buffer_Has_Space(), Buffer_Clear() * cache regex results for better performance * add "ends_on" (INI) property to SyntaxBlockDef * implement ends_on in the SyntaxHighlight modul * remove the check if the ends_on block is allowed inside the surrounding block. this leads to more flexibility and better performance * add additional (but just a single) test for the new feature * add Stack_CopyTo() * save open_blocks_at_begin and open_blocks_at_end to SyntaxHighlightingString instances ins SyntaxHighlighting_HighlightString() * Add Syntax Highlighting binding for TextBuffer (#87) * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * Add early-exit guards to table lookup functions (#80) * Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete() * Add CodeRabbit configuration template Added a comprehensive configuration template for CodeRabbit, including global settings, review settings, chat configurations, knowledge base settings, and code generation options. * add SyntaxHighlightingBinding to connect SyntaxHighlighting with the TextBuffer/TextLayout * fix some obvious bugs in SyntaxHighlighingBinding * fix memory leak * add basic test * add Stack_Size() * rename test function * add Stack_CopyTo() * save open_blocks_at_begin and open_blocks_at_end to SyntaxHighlightingString instances ins SyntaxHighlighting_HighlightString() * adept code to the changed SyntaxHighlighingString scheme * improve tests * Compare the open_blocks_at_begin with the open_blocks parameter for an early exit int SyntaxHighlighting_HighlightString() * add testcases and improve test framework's flexibility * fix coderabbit config * fix resizing in Stack_CopyTo() * add NULL guard to SyntaxHighlighting_HighlightString() * change multiline string to valid C string * change multiline strings to valid C strings * set shs->tags_capacity correctly in SyntaxHighlioghtingString_Create() * make argv0 available through Config * Add File_Exits(), File_OpenConfig() and File_OpenProjectFile() * change defautl width * fix wrong function calls * use new File_LoadConfig() function * remove unused variable * check if snprintf() truncated the path * fix function declaration of File_OpenProjectFile() * Implement a loader function to load a complete SyntaxHighlighting engine from an INI file Enable compile_commands.json and improve syntax handling (#88) * propagate PATH_MAX by file.h * create compile_commands.json (for correct using include paths in vscode) * add create and destroy functions to SyntaxHighlighting * SyntaxHighlighting now takes the ownership of the used SyntaxDefinition * must not destroy SyntaxDefinition anymore since this is handled by SyntaxHighlighting * fix bug in find_project_file() * implement the SyntaxHighlighting_LoadFromFile() function and tests * add NULL guard * Initialize error properly in SyntaxHighlighting_LoadFromFile() * guard againsst line == NULL and handle last_line == NULL in SyntaxHighlightingBinding_Update() * fix bug in concat_paths() * add test_definition_error to TEST_LIST * Integrate SyntaxHighlighting to the editor (#89) * wip * link to the data directory in the build directory * example syntax definitions * massive changes to get syntax highlighting working... need to optimization and better organisation, lot of work arounds * comment out TESTFILE definition * destroy SyntaxHighlighting instance properly * fix Table_Get() call in draw_visual_line() to use the correct key * improve highlighting * add Config_SetSyntax(), Config_GetSyntax() * use getopt() to parse cmd arguments, take syntax type from cmd arguments * Trigger a full SyntaxHighlighting update once in the beginning * add hint that byte offset is used * update screenshot * improve markdown highlighting * remove TESTFILE debugging stuff * handle the case that no highlighting is selected * deinitialize IniParser at ealry exit if c == NULL in Config_LoadIni() * remove double deinitialization of TextSelection in editor_destroy() * fix parse_arguments() and update print_help() * fix critical bug in Table_Set/Get(): change state of tombstones when reusing them * anchor section block start to newline * fix comment * fix off-by-one bug in Buffer_HasSpace() * guard against NULL in SyntaxHighlightingBinding_UpdateAll() * update syntax definitions * fix off-by-one bug in SyntaxHighlightingString_AddTag() * fix potential int overflow * add comment * minor change to const correctness in cpy_ptr * update readme * fix intendation
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores