Add Syntax Highlighting binding for TextBuffer - #87
Conversation
* add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool
* Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete()
Added a comprehensive configuration template for CodeRabbit, including global settings, review settings, chat configurations, knowledge base settings, and code generation options.
…TextBuffer/TextLayout
…gString instances ins SyntaxHighlighting_HighlightString()
…n early exit int SyntaxHighlighting_HighlightString()
WalkthroughThis PR adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Binding as SyntaxHighlightingBinding
participant Highlight as SyntaxHighlighting
participant TextBuf as TextBuffer
Caller->>Binding: Update(binding, line, last_line)
activate Binding
alt previous line not computed
Binding->>Binding: Update(prev_line, ...)
end
loop each line from line..last_line
Note over Binding: obtain open_blocks (from prev line end or NULL)
Binding->>Highlight: HighlightString(text, open_blocks)
activate Highlight
Highlight-->>Binding: const Stack* end_blocks
deactivate Highlight
alt current line == TextBuf current line
Binding->>TextBuf: MergeGap()
end
end
deactivate Binding
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
tests/test_syntax_textlayoutbindings.c (1)
48-58: Make allocation size expression robust to type changes.Use sizeof(*ptr) to avoid mismatch if the pointer type changes.
- fixture->lines = malloc(sizeof(Line*) * fixture->line_count); + fixture->lines = malloc(sizeof(*fixture->lines) * fixture->line_count);src/syntax/textlayoutbindings.h (1)
1-16: Optional: add C++ compatibility guards.Only if you expect C++ consumers.
#ifndef SYNTAX_TEXTLAYOUTBINDINGS_H #define SYNTAX_TEXTLAYOUTBINDINGS_H +#ifdef __cplusplus +extern "C" { +#endif ... void SyntaxHighlightingBinding_Update(SyntaxHighlightingBinding *binding, const Line *line, const Line *last_line); -#endif +#ifdef __cplusplus +} +#endif +#endifsrc/syntax/highlighting.c (2)
20-38: Avoid immediate realloc by initializing tags_capacity.You malloc initial capacity but leave tags_capacity=0, forcing a redundant realloc on first add.
shs->tags = NULL; shs->tags_count = 0; shs->tags_capacity = 0; ... shs->tags = malloc(sizeof(SyntaxHighlightingTag) * SHS_TAGS_INITIAL_CAPACITY); if (!shs->tags) { logFatal("Cannot allocate memory for SyntaxHighlightingString tags."); } + shs->tags_capacity = SHS_TAGS_INITIAL_CAPACITY;
186-203: Early-exit may skip recomputation on changed text bytes.If the line’s String contents mutate in place but the begin-stack is unchanged, returning cached end-stack yields stale tags. Ensure upstream invalidates sh->strings or add a lightweight content/version guard before early-exit.
Would you like me to scan for an existing text version/hash in the String type and wire that into the cache key?
src/common/stack.c (1)
31-37: Guard against capacity overflow when growing.Multiplication may overflow size_t for large capacities. Add a pre-check.
- size_t new_cap = stack->capacity == 0 ? STACK_INITIAL_CAPACITY : stack->capacity * STACK_GROW_FACTOR; + size_t new_cap = stack->capacity == 0 ? STACK_INITIAL_CAPACITY : stack->capacity * STACK_GROW_FACTOR; + if (stack->capacity && STACK_GROW_FACTOR > 0 && stack->capacity > SIZE_MAX / STACK_GROW_FACTOR) { + logFatal("Stack capacity overflow."); + } resize_capacity(stack, new_cap);src/syntax/highlighting.h (2)
63-65: Init/Deinit responsibilities for new Stack fields.Since these are by-value members, ensure SyntaxHighlightingString_Create calls Stack_Init for both, and Destroy calls Stack_Deinit to avoid UB/leaks.
108-111: Clarify return value lifetime.Returning a const Stack* implies the caller holds a reference into internal storage. Document that it stays valid until the next highlight of the same
text(or until the SH instance is destroyed).src/syntax/textlayoutbindings.c (2)
13-24: Defensive checks and NULL-return handling.Consider early NULL checks for binding/line and assert that SyntaxHighlighting_HighlightString never returns NULL; if it can, break to avoid propagating a NULL state.
26-51: Const-correctness and recursion depth.
- Use const for local pointers (
prev_line,open_blocks) to match APIs.- Replace recursion with an iterative back-scan to avoid deep recursion on long buffers.
- Fix comment typos.
Apply:
-void SyntaxHighlightingBinding_Update(SyntaxHighlightingBinding *binding, const Line *line, const Line *last_line) { - Line *prev_line = line->prev; - Stack *open_blocks = NULL; +void SyntaxHighlightingBinding_Update(SyntaxHighlightingBinding *binding, const Line *line, const Line *last_line) { + const Line *prev_line = line ? line->prev : NULL; + const Stack *open_blocks = NULL; @@ - SyntaxHighlightingString *shs = Table_Get(binding->sh->strings, &prev_line->text); + SyntaxHighlightingString *shs = Table_Get(binding->sh->strings, &prev_line->text); if (shs) { // use the open_blocks from the end of previous line open_blocks = &shs->open_blocks_at_end; } else { - // highlighting for the line is not calculated so far - // so run this function for thr previous line - SyntaxHighlightingBinding_Update(binding, prev_line, last_line); - return; + // highlighting for the previous line is not calculated so far + // walk backwards to find the nearest computed state + const Line *p = prev_line; + while (p) { + shs = Table_Get(binding->sh->strings, &p->text); + if (shs) { open_blocks = &shs->open_blocks_at_end; break; } + p = p->prev; + } } } if (line == binding->tl->tb->current_line) { - // this is super dirty cause the gap funcitonality is totally disabled this way!!! + // this is super dirty cause the gap functionality is totally disabled this way!!! // NEED A BETTER SOLUTION TextBuffer_MergeGap((TextBuffer*)binding->tl->tb); } @@ - update_lines(binding, line, last_line, open_blocks); + update_lines(binding, line, last_line, open_blocks); }Additionally, include the necessary header to make
SyntaxHighlightingStringvisible in this TU:// at top of file #include "syntax/highlighting.h"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.coderabbit.yaml(1 hunks)src/common/stack.c(3 hunks)src/common/stack.h(2 hunks)src/syntax/highlighting.c(3 hunks)src/syntax/highlighting.h(2 hunks)src/syntax/textlayoutbindings.c(1 hunks)src/syntax/textlayoutbindings.h(1 hunks)tests/test_syntax_highlighting.c(17 hunks)tests/test_syntax_textlayoutbindings.c(1 hunks)
🔇 Additional comments (10)
src/common/stack.h (1)
38-40: New Stack APIs look good.Interfaces are clear and align with intended semantics. No issues.
Also applies to: 48-49
src/syntax/textlayoutbindings.h (1)
1-16: API surface is clean and minimal.Struct and init/update functions look good for the binding.
src/common/stack.c (2)
77-80: Copy via Create+CopyTo looks good.
146-149: New Stack_Size accessor is fine.src/syntax/textlayoutbindings.c (2)
3-6: Binding init is straightforward.
8-11: Binding deinit is straightforward.tests/test_syntax_highlighting.c (4)
45-52: Helper for resolving block names looks good.
85-89: Good: asserting identity of returned end-state stack.This verifies the API contract that the return is the SHS’s end-state stack reference.
Consider adding a negative test ensuring the pointer changes after re-highlighting the same line with different input.
93-101: Nice use of Stack_Size; indexing order check is correct.
114-116: Clean teardown.Blocks table and temp stack are properly destroyed.
|
@coderabbitai review changes |
|
@defname: I'll review the changes in this PR. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/stack.c (1)
16-19: Include <stdlib.h> for malloc/realloc/free.Prevents implicit-declaration errors and mismatched types.
#include "stack.h" #include <string.h> +#include <stdlib.h> #include "logging.h"
♻️ Duplicate comments (5)
src/syntax/highlighting.c (1)
191-197: Good fix on NULL-deref; extend cache to handle implicit-root case.Guarding
stack_equalwithopen_blocks_at_beginprevents crashes. To also hit the fast path when callers pass NULL (implicit[root]), add a check that treats “NULL begin” as{root}and returns the cached end stack ifopen_blocks_at_beginis NULL andopen_blocks_at_beginstored equals[root].Suggested tweak:
- if (open_blocks_at_begin && stack_equal(&shs->open_blocks_at_begin, open_blocks_at_begin)) { - return &shs->open_blocks_at_end; - } + if (open_blocks_at_begin) { + if (stack_equal(&shs->open_blocks_at_begin, open_blocks_at_begin)) { + return &shs->open_blocks_at_end; + } + } else { + if (Stack_Size(&shs->open_blocks_at_begin) == 1 && + Stack_Peek(&shs->open_blocks_at_begin) == sh->def->root) { + return &shs->open_blocks_at_end; + } + }tests/test_syntax_textlayoutbindings.c (2)
81-99: C raw string literal replaced correctly.Switch to standard C string literal with escapes looks good.
1-7: Add standard headers for malloc/free and strlen.Required for strict C builds.
#include "acutest.h" #include "syntax/textlayoutbindings.h" #include "syntax/definition.h" #include "common/iniparser.h" #include "common/string.h" #include "document/textlayout.h" +#include <stdlib.h> +#include <string.h>src/common/stack.c (2)
96-103: Use sizeof dereference for copy/clear; safer and self-documenting.Switch from
sizeof(void *)tosizeof(*dst->items).- memcpy(dst->items, src->items, src->size * sizeof(void *)); + memcpy(dst->items, src->items, src->size * sizeof(*dst->items)); // set remainder to 0 if (dst->capacity > src->size) { - memset(dst->items + src->size, 0, (dst->capacity - src->size) * sizeof(void *)); + memset(dst->items + src->size, 0, (dst->capacity - src->size) * sizeof(*dst->items)); }
20-29: Fix realloc misuse to avoid pointer loss on OOM.Assigning realloc directly to
stack->itemscan lose the original pointer if realloc fails.static void resize_capacity(Stack *stack, size_t new_capacity) { if (!stack || stack->capacity >= new_capacity) { return; } - stack->items = realloc(stack->items, new_capacity * sizeof(void *)); - if (!stack->items) { + void **new_items = realloc(stack->items, new_capacity * sizeof(*stack->items)); + if (!new_items) { logFatal("Failed to reallocate stack items."); } - stack->capacity = new_capacity; + stack->items = new_items; + stack->capacity = new_capacity; }
🧹 Nitpick comments (2)
src/syntax/highlighting.c (1)
175-185: Avoid duplicating stack equality logic; consider promoting to stack API or add NULL-safety.
stack_equalis local and assumes non-NULL inputs. Either:
- Move this as
Stack_Equals(const Stack*, const Stack*)tosrc/common/stack.*, or- Make it NULL-safe if kept local (treat NULL vs non-NULL as unequal; two NULLs equal).
tests/test_syntax_highlighting.c (1)
96-101: Avoid reaching into Stack internals in tests.Accessing
open_blocks->items[...]ties tests to implementation details. Prefer an accessor (e.g.,Stack_Get(const Stack*, size_t)), or a small test-only helper to index from bottom/top.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.coderabbit.yaml(1 hunks)src/common/stack.c(3 hunks)src/syntax/highlighting.c(3 hunks)tests/test_syntax_highlighting.c(17 hunks)tests/test_syntax_textlayoutbindings.c(1 hunks)
🔇 Additional comments (3)
.coderabbit.yaml (3)
175-176: ✅ Schema violation fixed.The
base_branchesfield is now correctly defined as a YAML array matching the schema requirement, resolving the previous critical issue. The regex pattern".*"appropriately enables review on all branches for this template.
171-171: Note:draftsintentionally set totrue.The template sets
drafts: true(schema default isfalse), which is appropriate for an encouraging template configuration that promotes automatic reviews on draft PRs.
1-805: Comprehensive configuration template with proper schema alignment.The template provides:
- Well-organized sections with clear section headers (lines 5–805)
- Descriptive comments for each configuration field
- Inline defaults matching the schema specification
- Proper YAML indentation and syntax throughout
- Helpful placeholder examples for tool-specific config files (e.g., lines 372, 412, 447, 506, 533)
This serves as a solid foundation for repository-specific configuration customization.
| // 3. Check | ||
| SyntaxHighlightingString *shs0 = Table_Get(sh->strings, &fixture.lines[0]->text); | ||
| SyntaxHighlightingString *shs1 = Table_Get(sh->strings, &fixture.lines[1]->text); | ||
| TEST_CHECK(shs0 != NULL); | ||
| TEST_CHECK(shs0->text == &fixture.lines[0]->text); | ||
| TEST_CHECK(shs0->tags_count == 1); | ||
| TEST_CHECK(shs0->tags[0].byte_offset == 6); | ||
| TEST_CHECK(shs0->open_blocks_at_end.size == 2); | ||
| TEST_MSG("%zu", shs0->open_blocks_at_end.size); | ||
| TEST_CHECK(shs0->open_blocks_at_end.size == shs1->open_blocks_at_begin.size); | ||
| TEST_CHECK(Table_Get(sh->strings, &fixture.lines[1]->text) != NULL); |
There was a problem hiding this comment.
Fix potential NULL dereference of shs1 before checking it.
shs1 is used on Line 155 prior to verifying it’s non-NULL.
SyntaxHighlightingString *shs0 = Table_Get(sh->strings, &fixture->lines[0]->text);
SyntaxHighlightingString *shs1 = Table_Get(sh->strings, &fixture->lines[1]->text);
TEST_CHECK(shs0 != NULL);
+ TEST_ASSERT(shs1 != NULL);
TEST_CHECK(shs0->text == &fixture->lines[0]->text);
TEST_CHECK(shs0->tags_count == 1);
TEST_CHECK(shs0->tags[0].byte_offset == 6);
TEST_CHECK(shs0->open_blocks_at_end.size == 2);
TEST_MSG("%zu", shs0->open_blocks_at_end.size);
- TEST_CHECK(shs0->open_blocks_at_end.size == shs1->open_blocks_at_begin.size);
- TEST_CHECK(Table_Get(sh->strings, &fixture->lines[1]->text) != NULL);
+ TEST_CHECK(shs0->open_blocks_at_end.size == shs1->open_blocks_at_begin.size);
+ TEST_CHECK(Table_Get(sh->strings, &fixture->lines[1]->text) != NULL);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/test_syntax_textlayoutbindings.c around lines 146 to 156, shs1 is
dereferenced (used in the size comparison at line 155) before confirming it is
non-NULL; add an explicit null check for shs1 (e.g., TEST_CHECK(shs1 != NULL))
immediately after obtaining shs1 from Table_Get and before any access to
shs1->open_blocks_at_begin to avoid a potential NULL dereference, and if desired
adjust the order of TEST_CHECK calls so the null check runs prior to the size
comparison.
* 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
Performance Improvements
Refactor
Tests