Feat/Syntax Highlighting: Add module to generate Highlighting information - #85
Conversation
…lock end comes first
…_HighlightString()
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughAdds Stack lifecycle APIs (create/destroy/copy/is-empty), generalizes Table to accept generic pointer keys with customizable hash/cmp/copy/free hooks and new creators, implements a new SyntaxHighlighting subsystem with public APIs, adjusts SyntaxDefinition root/error handling, and adds tests for highlighting, definitions, and pointer-key tables. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant HL as SyntaxHighlighting
participant SHS as SyntaxHighlightingString
participant DEF as SyntaxDefinition
participant STACK as Block Stack
Caller->>HL: SyntaxHighlighting_HighlightString(text, open_blocks)
alt cached string exists
HL->>SHS: reuse SHS for text
else build or create
HL->>SHS: SyntaxHighlightingString_Create(text)
end
HL->>STACK: clone or init open_blocks
loop scan text positions
HL->>DEF: find_first_child(current_block, text_at_pos)
alt enter child
HL->>SHS: add begin tag (offset)
HL->>STACK: push child
else check end
HL->>DEF: find_end_of_block(current_block, text_at_pos)
alt end match
HL->>SHS: add end tag (offset)
HL->>STACK: pop block
else advance pos
end
end
end
HL->>Caller: return STACK (open blocks at end)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Rationale: Fundamental API redesign in the table subsystem (key type, ownership, multiple factory functions), a substantial new syntax-highlighting subsystem with nontrivial traversal/state logic, plus cross-cutting tests and definition changes—heterogeneous, dense, and requiring careful semantic review. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/common/stack.c (1)
31-35: Use a temp pointer for realloc to avoid clobbering on failure.Assigning realloc directly to stack->items risks losing the old pointer on OOM (even if logFatal aborts). Use a temporary, then assign.
- stack->items = realloc(stack->items, stack->capacity * sizeof(void *)); - if (!stack->items) { + void **new_items = realloc(stack->items, stack->capacity * sizeof(void *)); + if (!new_items) { logFatal("Failed to reallocate stack items."); } + stack->items = new_items;src/common/table.c (1)
121-132: Preserve custom key functions during resize.Using Table_Create() breaks pointer/custom-key tables (e.g., strdup on pointers). Recreate with the same hooks.
- Table *new_table = Table_Create(); + Table *new_table = Table_CreateCustom( + table->hash_func, + table->key_cmp_func, + table->key_copy_func, + table->key_free_func);
🧹 Nitpick comments (23)
src/common/stack.h (2)
35-39: Use (void) in no‑arg prototype and document copy semantics.
- Prefer C-style no-arg:
(void)to avoid old K&R ambiguity.- Clarify whether
Stack_Copyperforms shallow vs deep copy ofitems.Apply:
-Stack *Stack_Create(); +Stack *Stack_Create(void);Optionally add a brief doc comment here stating: “Copy is shallow; item pointers are not duplicated.”
46-47: Consider making IsEmpty inline (optional).If this remains a simple
size == 0check, consider astatic inlinein the header for zero call overhead; otherwise keep in .c for ABI stability. Also ensure it’s null-safe or documented as requiring non‑NULL.src/syntax/definition.h (1)
45-47: Align minimal example with root special‑case.Docs now say root needn’t define
start/end, but the “Minimal example” above still setsstart. Add a second example (or amend text) showing root with onlyallowed_blocks, clarifying both forms are supported.tests/test_syntax_definition.c (1)
141-149: LGTM: end‑regex error case; consider consistent quoting.Works as intended. Optional: use consistent quoting for
startacross cases to reduce noise in diffs.tests/test_table.c (1)
118-129: Avoid int→pointer casts in tests; use uintptr_t for portability.On some platforms this triggers
-Wint-to-pointer-cast. Prefer(void*)(uintptr_t)N. Add<stdint.h>include.Apply:
+#include <stdint.h> ... - Table_Set(table, (void*)1, strdup("Foobar"), free); - Table_Set(table, (void*)2, strdup("Blub"), free); + Table_Set(table, (void*)(uintptr_t)1, strdup("Foobar"), free); + Table_Set(table, (void*)(uintptr_t)2, strdup("Blub"), free);src/syntax/definition.c (2)
78-83: Root regex comments are misleading; consider clearer patterns.
"^"doesn’t “match always”; it’s BOL. If the goal is “immediate zero‑width” consider()(matches empty) for start;a^for never‑match end is fine. At minimum, fix comments to state “BOL anchor”.Apply (comment‑only) or switch to true empty‑match:
- regcomp(&block->start, "^", REG_EXTENDED); // matches always without consuming + regcomp(&block->start, "^", REG_EXTENDED); // BOL anchor; zero-width at startOptionally:
- regcomp(&block->start, "^", REG_EXTENDED); + regcomp(&block->start, "()", REG_EXTENDED); // zero-width, matches anywhere
122-124: Comment doesn’t match the pattern behavior.Pattern
"^"is not “matches everything without consuming”; it’s BOL. Sinceonly_startremains true, this is just a sentinel for saferegfree. Update the comment to reflect that, or switch to an impossible pattern likea^for clarity.- regcomp(&block->end, "^", REG_EXTENDED); // matches everything without consuming + regcomp(&block->end, "^", REG_EXTENDED); // sentinel for regfree; unused when only_start==truesrc/common/stack.c (2)
37-42: Add a NULL guard in Stack_Init for consistency with other APIs.Other functions already guard NULL; do the same here.
-void Stack_Init(Stack *stack) { +void Stack_Init(Stack *stack) { + if (!stack) { + return; + }
127-129: Null-safe Stack_IsEmpty.Avoid potential NULL deref; align with other guards.
-bool Stack_IsEmpty(const Stack *stack) { - return stack->size == 0; -} +bool Stack_IsEmpty(const Stack *stack) { + return !stack || stack->size == 0; +}tests/test_syntax_highlighting.c (2)
21-28: Unused helper: make static or remove.build_blocks_table() is not referenced; compilers will warn. Either remove or mark static until needed.
-Table *build_blocks_table(SyntaxDefinition *def) { +static Table *build_blocks_table(SyntaxDefinition *def) {
60-62: Fix test message wording.Double negative in message.
- TEST_MSG("Expected open_blocks to not be not empty."); + TEST_MSG("Expected open_blocks to not be empty.");src/common/table.c (1)
244-251: Generic error message in Table_Set.Not all keys are strings.
- if (slot->key == NULL) { - logFatal("No memory for string copy in Table_Set()."); - } + if (!slot->key) { + logFatal("No memory to copy key in Table_Set()."); + }src/common/table.h (1)
81-90: Clarify non-NULL requirements for custom key hooks.Document (or assert) that hash_func, key_cmp_func, and key_copy_func must be non-NULL; key_free_func may be NULL.
src/syntax/highlighting.c (4)
1-2: Unify logging include and include required headers explicitly.Other modules include "logging.h". Also include <regex.h> since regmatch_t/regexec are used.
-#include "highlighting.h" -#include "common/logging.h" +#include "highlighting.h" +#include "logging.h" +#include <regex.h>
20-37: Initialize tags_capacity at creation to avoid immediate realloc.Set capacity once and skip the first redundant grow.
shs->text = text; - shs->tags = NULL; - shs->tags_count = 0; - shs->tags_capacity = 0; + shs->tags = NULL; + shs->tags_count = 0; + shs->tags_capacity = SHS_TAGS_INITIAL_CAPACITY; Stack_Init(&shs->open_blocks_at_end); - shs->tags = malloc(sizeof(SyntaxHighlightingTag) * SHS_TAGS_INITIAL_CAPACITY); + shs->tags = malloc(sizeof(SyntaxHighlightingTag) * SHS_TAGS_INITIAL_CAPACITY);
102-122: Avoid shadowing the output parameter in find_first_child.Shadowing
matchis confusing and easy to break; use a different local.- for (size_t i=0; i<current->children_count; i++) { + for (size_t i=0; i<current->children_count; i++) { const SyntaxBlockDef *child = current->children[i]; - regmatch_t match; - if (regexec(&child->start, str, 1, &match, 0) == 0) { - if (!had_match || match.rm_so < first_match.rm_so) { - first_match = match; + regmatch_t cur_match; + if (regexec(&child->start, str, 1, &cur_match, 0) == 0) { + if (!had_match || cur_match.rm_so < first_match.rm_so) { + first_match = cur_match; first_block = child; had_match = true; } } }
133-207: Consider persisting open blocks into shs->open_blocks_at_end or remove the field.You initialize and deinit shs->open_blocks_at_end but never populate it. Either store a copy of the final stack for caching, or drop the field to avoid confusion.
src/syntax/highlighting.h (6)
50-52: Are Tag_Init/Tag_Deinit necessary?The tag holds only non-owning pointers and a size_t. If these are no-ops, consider removing them from the public API to keep it lean, or document their effects (e.g., zeroing).
Can you confirm whether these do more than memset/zero? If not, I can send a follow-up patch to drop them.
66-68: Namespace or type the public constants.Macros are fine, but consider namespacing or typed constants to avoid collisions.
-#define SHS_TAGS_INITIAL_CAPACITY 16 -#define SHS_TAGS_GROW_FACTOR 2 +enum { + SHS_TAGS_INITIAL_CAPACITY = 16, + SHS_TAGS_GROW_FACTOR = 2 +};
69-74: Document allocation/error semantics.
SyntaxHighlightingString_Createshould specify it returns NULL on allocation failure.SyntaxHighlightingString_AddTagshould state behavior on reallocation failure (return bool? abort?).If you want, I can propose a minimal change to return
boolfrom AddTag and Clear to signal failures.
87-88: Add C++ compatibility guards.Expose C symbols when included from C++.
#define SYNTAX_HIGHLIGHTING_H + +#ifdef __cplusplus +extern "C" { +#endif @@ void SyntaxHighlighting_Deinit(SyntaxHighlighting *sh); @@ Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *hl, const String *text, const Stack *open_blocks); +#ifdef __cplusplus +} +#endifAlso applies to: 92-92, 106-106
106-106: Optional: unify parameter names (hl→sh).For consistency with Init/Deinit.
-Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *hl, const String *text, const Stack *open_blocks); +Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *sh, const String *text, const Stack *open_blocks);
39-41: Minor wording tweak.Consider clarifying “end of a block is treated as a new beginning of the surrounding block” with an example (e.g., how nested blocks are represented) to avoid ambiguity for integrators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/common/stack.c(3 hunks)src/common/stack.h(1 hunks)src/common/table.c(12 hunks)src/common/table.h(4 hunks)src/syntax/definition.c(3 hunks)src/syntax/definition.h(1 hunks)src/syntax/highlighting.c(1 hunks)src/syntax/highlighting.h(1 hunks)tests/test_syntax_definition.c(2 hunks)tests/test_syntax_highlighting.c(1 hunks)tests/test_table.c(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
src/syntax/highlighting.h
[error] 30-30: 'stdint.h' file not found
(clang-diagnostic-error)
src/common/table.h
[error] 33-33: 'stdint.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (5)
tests/test_syntax_definition.c (2)
137-140: LGTM: start‑regex error case.Good negative coverage for malformed start regex.
179-179: Nice: TEST_CASE labeling per INI string.Improves failure localization across the loop.
tests/test_table.c (1)
136-136: LGTM: added test to suite.Pointer‑key coverage plugged into TEST_LIST.
src/syntax/definition.c (1)
62-63: Unconditional regfree ofendis safe
All validSyntaxBlockDef_FromTablepaths—root special-case (lines 78–83), end-regex compile (lines 115–118), and default end compile (lines 122–124)—invokeregcomp(&block->end,…)before returning a non-NULL block, soregfree(&block->end)in the destructor is always paired with a prior compile.src/syntax/highlighting.h (1)
55-65: Clarify ownership, ordering, and deinitialization for SyntaxHighlightingString.
textis not owned by the struct.tagsbuffer is owned and must be freed in SyntaxHighlightingString_Destroy (and reset in Clear).tagsentries should be kept sorted bybyte_offset.open_blocks_at_endmust be deinitialized (e.g. via StackDestroy) in both Clear and Destroy — please verify these functions reset/free all fields.
…ter in the Table module
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/common/table.c (1)
121-132: Preserve custom key semantics during resize.
increase_capacitybuilds the new table withTable_Create(), hardwiring string keys. Pointer-key or custom-key tables will break (bad hashing/copying/ownership). Construct the new table with the same function pointers.- // allocate memory for the increased table - Table *new_table = Table_Create(); - new_table->capacity = table->capacity == 0 ? TABLE_INITIAL_CAPACITY : table->capacity * TABLE_GROWTH_FACTOR; + // allocate memory for the increased table (preserve key semantics) + Table *new_table = Table_CreateCustom( + table->hash_func, + table->key_cmp_func, + table->key_copy_func, + table->key_free_func); + new_table->capacity = table->capacity == 0 ? TABLE_INITIAL_CAPACITY : table->capacity * TABLE_GROWTH_FACTOR;
♻️ Duplicate comments (6)
src/common/table.c (1)
146-148: Good: avoid NULL key_free_func deref.src/syntax/highlighting.h (4)
21-26: Polish “Limitations” docs (typos/clarity).- * Regular expressions that mark the begin or the end of an block are not tested across different - * Strings. If for example a keyword starts in one string and continues in another one, even if - * they are parsed in the correct order, the keyword will not be recognized. - * So make sure the `text` parts are sperarated in a compatible way (like by newlines or similar). + * Regular expressions that mark the beginning or end of a block are not tested across different + * String instances. For example, if a keyword starts in one string and continues in another, even if + * parsed in the correct order, it will not be recognized. + * Make sure the `text` parts are separated on compatible boundaries (e.g., by newlines).
44-48: Avoid reserved leading-underscore struct tags; keep public typedef stable.-typedef struct _SyntaxHighlightingTag { +typedef struct SyntaxHighlightingTag_ { const String *text; //< pointer to the string this tag is for size_t byte_offset; //< byte offset where the tag occurs const SyntaxBlockDef *block; //< pointer to the block this tag is for } SyntaxHighlightingTag;Apply similarly to
_SyntaxHighlightingStringand_SyntaxHighlightingtags below.
79-81: Clarify Table ownership semantics in comments.- Table *strings; //< Table of (String -> SyntaxHighlightingString*) elements (holds the ownership of the SyntaxHighlightingString's) + Table *strings; //< Table of (String* -> SyntaxHighlightingString*) built via Table_CreatePtr(): + //< - Keys (String*) are NOT owned/freed by the table (pointer-identity hash; no key-destroy). + //< - Values (SyntaxHighlightingString*) ARE owned and destroyed via the destructor passed to Table_Set.
97-105: Doc nits: param name consistency, typos, and return ownership.- * Add a `SyntaxHighlightingString`to `hl->strings` or update an existing one. + * Add a `SyntaxHighlightingString` to `sh->strings` or update an existing one. @@ - * @param hl The `SyntaxHighlighting` instance to use. - * @param text The text to hightlight. - * @param open_blocks An pointer to a `Stack` instance that holds the open blocks at the beginning of string. If NULL it's assumed that the root block is current. + * @param sh The `SyntaxHighlighting` instance to use. + * @param text The text to highlight. + * @param open_blocks A pointer to a `Stack` holding the open blocks at the beginning of `text`. If NULL, the root block is assumed current. @@ - * A new created `Stack` containing all open blocks at the end of `text`. + * A newly created `Stack` containing all open blocks at the end of `text`. + * The caller owns the returned stack and must deinitialize/free it.src/common/stack.c (1)
71-97: Fix capacity assignment in Stack_Copy; guard zero-byte memcpy.
copy->capacityis set tosrc->capacityeven when you allocated withcap(fallback for 0). This desynchronizes metadata and buffer, breaking push/resize later. Also avoid memcpy on size==0.Stack *Stack_Copy(const Stack *src) { @@ - size_t cap = src->capacity ? src->capacity : STACK_INITIAL_CAPACITY; // handle capacity == 0 + size_t cap = src->capacity ? src->capacity : STACK_INITIAL_CAPACITY; // handle capacity == 0 @@ - memcpy(copy->items, src->items, src->size * sizeof(void *)); + if (src->size) { + memcpy(copy->items, src->items, src->size * sizeof(void *)); + } @@ - copy->size = src->size; - copy->capacity = src->capacity; + copy->size = src->size; + copy->capacity = cap;
🧹 Nitpick comments (12)
src/syntax/definition.c (1)
237-237: Typo in log message.“Potential rrror” → “Potential error”.
- logFatal("Potential rrror in String_Split()."); + logFatal("Potential error in String_Split().");src/common/stack.c (1)
136-138: Make Stack_IsEmpty null-safe (consistent with other APIs).-bool Stack_IsEmpty(const Stack *stack) { - return stack->size == 0; -} +bool Stack_IsEmpty(const Stack *stack) { + return !stack || stack->size == 0; +}src/common/table.c (1)
246-249: Message wording now generic (not only “string copy”).Optional: rephrase to “key copy” for non-string tables.
- logFatal("No memory for string copy in Table_Set()."); + logFatal("No memory for key copy in Table_Set().");src/syntax/highlighting.c (4)
21-37: Initialize tags_capacity when preallocating.You allocate with SHS_TAGS_INITIAL_CAPACITY but leave
tags_capacity=0, forcing an immediate redundant realloc on first add.shs->tags = NULL; shs->tags_count = 0; - shs->tags_capacity = 0; + shs->tags_capacity = 0; @@ - shs->tags = malloc(sizeof(SyntaxHighlightingTag) * SHS_TAGS_INITIAL_CAPACITY); + 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;
67-75: Off-by-one in capacity check.
if (tags_count + 1 >= tags_capacity)wastes one slot. Prefer==(or>=without +1).- if (shs->tags_count + 1 >= shs->tags_capacity) { + if (shs->tags_count == shs->tags_capacity) { increase_tags_capacity(shs); }
134-145: Typo: “infomation” → “information”.- // check if there is already old infomation about text in the table + // check if there is already old information about text in the table
77-83: Consider storing open blocks into shs->open_blocks_at_end (or remove the field).Right now the field is never updated; results only come back via the returned Stack. Either populate
shs->open_blocks_at_end(e.g., clear then push pointers from the returned stack) or drop the field to avoid confusion.If you want, I can draft a small helper
void Stack_Assign(Stack *dst, const Stack *src)to copy contents without heap churn.src/syntax/highlighting.h (3)
63-64: Same reserved-identifier fix for SyntaxHighlightingString tag.-typedef struct _SyntaxHighlightingString { +typedef struct SyntaxHighlightingString_ {
78-81: Same reserved-identifier fix for SyntaxHighlighting tag.-typedef struct _SyntaxHighlighting { +typedef struct SyntaxHighlighting_ {
75-76: **Stray “/match” token.-/**match +/**tests/test_syntax_highlighting.c (2)
292-314: Remove commented-out code.Line 293 contains commented-out code that should be removed for cleaner test code.
Apply this diff:
void generate_random_string(char *out, size_t max_len, const char *tokens[], size_t token_count) { - //size_t len = rand() % (max_len/2) + (max_len/2) - 1; size_t len = rand() % max_len;
24-31: Remove unused functionbuild_blocks_table.The function is never called anywhere in the codebase and should be removed as dead code. The verification search found only the definition itself with no call sites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/common/stack.c(3 hunks)src/common/table.c(12 hunks)src/syntax/definition.c(3 hunks)src/syntax/definition.h(2 hunks)src/syntax/highlighting.c(1 hunks)src/syntax/highlighting.h(1 hunks)tests/test_syntax_highlighting.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/syntax/definition.h
🧰 Additional context used
🪛 Clang (14.0.6)
src/syntax/highlighting.h
[error] 30-30: 'stddef.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (10)
src/common/stack.c (1)
16-18: Include <string.h>: good addition.src/common/table.c (1)
180-181: Good: pass table->key_free_func into TableSlot_Deinit.tests/test_syntax_highlighting.c (8)
1-9: Headers issue resolved.The missing headers flagged in the previous review have been added correctly.
10-22: LGTM!The helper function correctly handles INI parsing, error checking, and cleanup.
33-41: LGTM!The test case structure is well-designed for data-driven testing with reasonable fixed-size arrays.
93-130: LGTM!The basic test case is well-structured with proper setup, validation, and cleanup.
132-256: LGTM!The comprehensive test suite covers a good variety of scenarios including edge cases, nesting, and empty strings.
259-278: LGTM!The open blocks test appropriately validates that unclosed syntax blocks are tracked correctly.
348-354: LGTM!Test list is properly configured with all test functions registered and correctly NULL-terminated.
317-346: No action required on NULL parameter handling.The function properly handles NULL by explicitly checking and creating a fresh stack initialized with the root block (lines 148–154, src/syntax/highlighting.c). This is the intended behavior, and the test correctly validates it. The non-deterministic seeding is standard practice for stress tests.
| if (strcmp(name, "root") == 0) { | ||
| regcomp(&block->start, "^", REG_EXTENDED); // matches always without consuming | ||
| regcomp(&block->end, "a^", REG_EXTENDED); // matches never | ||
| block->only_start = false; | ||
| return block; | ||
| } |
There was a problem hiding this comment.
Root end-regex “never match” sentinel is unsafe; avoid regex for root-ending entirely.
Pattern "a^" can match literally “a^” in POSIX ERE (caret is special only at start), so root may spuriously “end”. Prefer to bypass end matching for the root inside the highlighter instead of relying on a regex sentinel. See proposed change in highlighting.c to treat current == sh->def->root as non-ending.
Apply in src/syntax/highlighting.c (see that file’s comment “Root never ends” for the diff).
🤖 Prompt for AI Agents
In src/syntax/definition.c around lines 78 to 83, the root block currently
compiles a bogus end regex ("a^") which is unsafe; remove the regcomp call for
block->end and do not create an end regex for the root, and mark the block as
start-only by setting block->only_start = true so the highlighter treats the
root as never-ending (adjust any related initialization if necessary to avoid
using an uninitialized end regex).
| else { | ||
| regcomp(&block->end, "^", REG_EXTENDED); // matches everything without consuming | ||
| } |
There was a problem hiding this comment.
Defaulting end to “^” causes zero-length matches → infinite loop.
Compiling block->end to “^” for start-only blocks makes find_end_of_block() match immediately with length 0, so offset never advances and the loop spins. Fix by not considering end for only_start blocks in the highlighter.
Apply in src/syntax/highlighting.c (see that file’s comment “Ignore end for start‑only blocks” for the diff).
🤖 Prompt for AI Agents
In src/syntax/definition.c around lines 122-124, compiling block->end to "^" for
start-only blocks creates zero-length matches and causes infinite loops;
instead, when block->only_start (or equivalent flag) is true do not compile or
assign an end regex (leave block->end unset/marked absent). Then update
src/syntax/highlighting.c where blocks are iterated so that find_end_of_block()
and any end-matching logic explicitly skip/ignore the end regex for start-only
blocks (use the existing "Ignore end for start-only blocks" comment as the
insertion point), relying only on start detection for these blocks. Ensure any
cleanup or free logic accounts for an absent/uncompiled end regex.
| static bool find_end_of_block(const char *str, const SyntaxBlockDef *current, regmatch_t *match) { | ||
| regmatch_t end_match; | ||
| if (regexec(¤t->end, str, 1, &end_match, 0) == 0) { | ||
| *match = end_match; | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Ignore end for start-only blocks to prevent zero-length loops.
Blocks with only_start=true should never be considered to end. This avoids immediate 0-length matches (e.g., defaulted “^”).
static bool find_end_of_block(const char *str, const SyntaxBlockDef *current, regmatch_t *match) {
+ if (current->only_start) {
+ return false;
+ }
regmatch_t end_match;
if (regexec(¤t->end, str, 1, &end_match, 0) == 0) {
*match = end_match;
return true;
}
return false;
}🤖 Prompt for AI Agents
In src/syntax/highlighting.c around lines 124 to 131, the function
find_end_of_block must skip attempting to match the block end when the
SyntaxBlockDef has only_start==true to avoid zero-length immediate matches;
modify the function to first check if current->only_start is true and return
false immediately (leaving match unchanged), otherwise proceed to call regexec
and assign end_match to *match on success.
| Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *sh, const String *text, const Stack *open_blocks_at_begin) { | ||
| // check if there is already old infomation about text in the table | ||
| SyntaxHighlightingString *shs = Table_Get(sh->strings, text); | ||
| if (shs) { | ||
| // clear it if so | ||
| SyntaxHighlightingString_Clear(shs); | ||
| } | ||
| else { | ||
| // otherwise create and store | ||
| shs = SyntaxHighlightingString_Create(text); | ||
| Table_Set(sh->strings, text, shs, (void(*)(void*))SyntaxHighlightingString_Destroy); | ||
| } | ||
|
|
||
| // create a working copy of the stack | ||
| Stack *open_blocks; | ||
| if (open_blocks_at_begin) { | ||
| open_blocks = Stack_Copy(open_blocks_at_begin); | ||
| } | ||
| else { | ||
| open_blocks = Stack_Create(); | ||
| Stack_Push(open_blocks, sh->def->root); | ||
| } | ||
|
|
||
| // iterate over the string | ||
| size_t offset = 0; | ||
| for (;;) { | ||
| // take the current block from the stack (but keep it there) | ||
| const SyntaxBlockDef *current_block = (SyntaxBlockDef*)Stack_Peek(open_blocks); | ||
|
|
||
| // find the first child block | ||
| regmatch_t child_match; | ||
| const SyntaxBlockDef *child = find_first_child(text->bytes + offset, current_block, &child_match); | ||
| regmatch_t end_match; | ||
| bool end_found = find_end_of_block(text->bytes + offset, current_block, &end_match); | ||
|
|
||
| if (child && (!end_found || child_match.rm_so < end_match.rm_so)) { | ||
| // if there is a child block found create and add a tag to SyntaxHighlightingString tag list | ||
| SyntaxHighlightingTag tag; | ||
| tag.text = text; | ||
| tag.byte_offset = offset + child_match.rm_so; | ||
| tag.block = child; | ||
| SyntaxHighlightingString_AddTag(shs, tag); | ||
|
|
||
| // increase the offset to the end of the match | ||
| offset += child_match.rm_eo; | ||
|
|
||
| // push the child to the stack to continue with it in the next iteration | ||
| Stack_Push(open_blocks, (void*)child); | ||
| continue; | ||
| } | ||
| // no child block found | ||
| // so check if the last block ends | ||
| if (end_found) { | ||
| // end of block found so remove it from stack | ||
| Stack_Pop(open_blocks); // removes current (which was just peeked before) | ||
|
|
||
| // and add a tag for the (new) begin of the surrounding block | ||
| SyntaxHighlightingTag tag; | ||
| tag.text = text; | ||
| tag.byte_offset = offset + end_match.rm_eo; // the current block goes behind the match of its end | ||
| tag.block = Stack_Peek(open_blocks); // new current block | ||
| SyntaxHighlightingString_AddTag(shs, tag); | ||
|
|
||
| // increase offset | ||
| offset += end_match.rm_eo; | ||
| continue; | ||
| } | ||
| // neither the beginning of a new block nor the end of the current block found | ||
| return open_blocks; | ||
| } | ||
|
|
||
| // if this happens there is an error in the SyntaxDefinition. | ||
| // the end pattern of the root block must *never* match | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Guarantee progress; handle root; avoid infinite loops on zero-length matches.
- Ensure loop progresses when a regex matches length 0 (child or end).
- Root must never “end”; bypass end-check when
current == sh->def->root.
@@
- bool end_found = find_end_of_block(text->bytes + offset, current_block, &end_match);
+ bool end_found = (current_block == sh->def->root)
+ ? false
+ : find_end_of_block(text->bytes + offset, current_block, &end_match);
@@
- // increase the offset to the end of the match
- offset += child_match.rm_eo;
+ // increase the offset to the end of the match; guarantee progress
+ size_t prev = offset;
+ offset += child_match.rm_eo;
+ if (offset == prev) {
+ // zero-length match: advance by one byte if possible
+ if (text->bytes[offset] == '\0') {
+ return open_blocks;
+ }
+ offset++;
+ }
@@
- // increase offset
- offset += end_match.rm_eo;
+ // increase offset; guarantee progress
+ size_t prev2 = offset;
+ offset += end_match.rm_eo;
+ if (offset == prev2) {
+ if (text->bytes[offset] == '\0') {
+ return open_blocks;
+ }
+ offset++;
+ }🤖 Prompt for AI Agents
In src/syntax/highlighting.c around lines 133 to 207, the main loop can stall on
zero-length regex matches and may incorrectly treat the root block as ending;
ensure progress and ignore root ends: when considering a child match or end
match, detect zero-length matches (rm_eo == rm_so) and in that case advance
offset by 1 byte (or by 1 if at EOF, clamp to text length) before continuing to
avoid infinite loops; also skip the end_found branch when current_block ==
sh->def->root so the root’s end pattern is never applied; finally ensure offset
never exceeds text length and return open_blocks when the scan completes.
| TEST_CHECK(!Stack_IsEmpty(open_blocks)); | ||
| TEST_MSG("Expected open_blocks to not be not empty."); |
There was a problem hiding this comment.
Fix double negative in error message.
The error message contains "not be not empty" which is confusing.
Apply this diff:
- TEST_MSG("Expected open_blocks to not be not empty.");
+ TEST_MSG("Expected open_blocks to not be empty.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST_CHECK(!Stack_IsEmpty(open_blocks)); | |
| TEST_MSG("Expected open_blocks to not be not empty."); | |
| TEST_CHECK(!Stack_IsEmpty(open_blocks)); | |
| TEST_MSG("Expected open_blocks to not be empty."); |
🤖 Prompt for AI Agents
In tests/test_syntax_highlighting.c around lines 63-64, the TEST_MSG text
contains a double negative "Expected open_blocks to not be not empty."; change
that message to a clear single-negative phrasing such as "Expected open_blocks
to be non-empty." or "Expected open_blocks to not be empty." by replacing the
string literal passed to TEST_MSG accordingly.
* 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