Feat/71 Syntax Highlighting - #90
Conversation
* 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
…tion (#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
* 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
…gString instances ins SyntaxHighlighting_HighlightString()
* 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()
…ame/clieditor into feat/71-syntax-highlighting
…ine 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
* 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
WalkthroughAdds a full INI-based syntax-highlighting subsystem (definition, loader, highlighter, textlayout binding) and editor integration; refactors Table to use generic void* keys with ownership callbacks; adds Buffer/Stack/Config/File utilities, syntax data files, tests, CMake updates, and README rewrite. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main.c
participant Loader as syntax/loader
participant File as io/file
participant Parser as iniparser
participant Def as syntax/definition
participant Highlight as syntax/highlighting
participant Editor as editor
Main->>Main: Config_Init(argv0)
Main->>Loader: SyntaxHighlighting_LoadFromFile(format, &err)
Loader->>File: File_OpenProjectFile(path)
File-->>Loader: file contents / NULL
Loader->>Parser: IniParser_Parse(contents)
Parser-->>Loader: Table / parse error
Loader->>Def: SyntaxDefinition_FromTable(Table)
Def-->>Loader: SyntaxDefinition / definition error
Loader->>Highlight: SyntaxHighlighting_Create(def)
Highlight-->>Loader: SyntaxHighlighting
Loader-->>Main: SyntaxHighlighting (or NULL + error)
Main->>Editor: Editor->sh_binding <- SyntaxHighlighting
Editor->>Highlight: SyntaxHighlighting_HighlightString(text, open_blocks)
Highlight-->>Editor: tags + open_blocks_at_end
sequenceDiagram
participant User as Caller
participant Table as Table API
User->>Table: Table_CreatePtr()
Table-->>User: Table (with ptr callbacks)
User->>Table: Table_Set(table, key_ptr, value)
Table->>Table: hash_func -> find_slot -> key_copy_func -> store
User->>Table: Table_Get(table, key_ptr)
Table->>Table: hash_func -> key_cmp_func -> return value
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: 22
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/common/string.h (1)
34-36: Header should include sys/types.h for ssize_t.This header declares functions using ssize_t but doesn’t include its definition. Make the header self-contained.
#include <stdlib.h> #include <stdbool.h> +#include <sys/types.h> // ssize_tsrc/common/config.c (1)
69-76: Leak on parse failure inConfig_LoadIni.Call
IniParser_Deinit(&ini)before returning whenIniParser_Parse()fails.Table *c = IniParser_Parse(&ini); if (!c) { - return; + IniParser_Deinit(&ini); + return; }src/widgets/components/editor.c (2)
39-44: Remove duplicate TextSelection_DeinitTextSelection_Deinit(&editor->ts) is called twice (Lines 40 and 43). Drop one to avoid confusion.
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); }
69-106: Add bounds check for syntax tag lookup and fix redundant assignmentThe code assumes
chpoints intoline->src->text.byteswhen computing byte offsets, butVisualLine_GetChar()can return pointers fromline->gap->textwhen a gap exists on the current line—causing undefined behavior. Additionally, line 116 has a redundant assignment:size_t byte_offset = byte_offset = ...Apply safe bounds checking before pointer arithmetic and clean up the assignment. Requires adding
#include <string.h>forstrlen().#include "common/utf8_helper.h" +#include <string.h> #include "document/textedit.h" @@ SyntaxHighlighting *sh = editor->sh_binding.sh; SyntaxHighlightingString *shs = !sh ? NULL : Table_Get(sh->strings, &line->src->text); @@ - // 2 --- Draw characters loop + // 2 --- Draw characters loop + const char *src_begin = line->src->text.bytes; + const char *src_end = src_begin ? src_begin + strlen(src_begin) : NULL; for (int i=0; i<line->length; 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; - - const SyntaxHighlightingTag *tag = SyntaxHighlightingString_GetTag(shs, byte_offset); - if (tag) { - canvas->current_style.fg = tag->block->color; - line_style.fg = tag->block->color; - } + // Only look up tags when character pointer lies within the source text buffer. + if (shs && src_begin && src_end && ch >= src_begin && ch < src_end) { + size_t byte_offset = (size_t)(ch - src_begin); + const SyntaxHighlightingTag *tag = SyntaxHighlightingString_GetTag(shs, byte_offset); + if (tag) { + canvas->current_style.fg = tag->block->color; + line_style.fg = tag->block->color; + } + }src/main.c (2)
52-85: Fix argument parsing: avoid pre-getopt filename assignment and unreachable code.Current logic sets filename to argv[1] even when it's an option and calls help multiple times. Simplify around getopt and handle remaining args cleanly.
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) { + Config_SetFilename(NULL); + int opt; + while ((opt = getopt(argc, argv, "hs:")) != -1) { switch (opt) { case 'h': print_help(argv[0]); - return; + return; // unreachable after exit(0), but keeps intent clear 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]); + // remaining non-option args: first one is the filename + if (optind < argc) { + Config_SetFilename(argv[optind]); + return; + } + print_help(argv[0]); }
99-118: Cast entry to const char before strcmp.*entry is a void* user pointer; strict C requires explicit cast for strcmp.
- if (strcmp(entry, "exit") == 0) { + if (strcmp((const char*)entry, "exit") == 0) { exit(0); } - if (strcmp(entry, "save") == 0) { + if (strcmp((const char*)entry, "save") == 0) { File *file = File_Open(Config_GetFilename(), FILE_ACCESS_WRITE);src/common/table.c (1)
237-263: Critical: inserting into a tombstone never sets key/state (corrupts table).When
find_slotreturns a TOMBSTONE for a new key,Table_Setdoesn’t setslot->stateto USED nor copy the key. This can also makeTable_Getreturn a value from a tombstone. Fix by treating EMPTY or TOMBSTONE identically on insert, and tighten the state check inTable_Get.Apply:
void Table_Set(Table *table, const void *key, void *value, void (*destructor)(void *value)) { @@ - if (slot->state == TABLE_SLOT_EMPTY) { + if (slot->state != TABLE_SLOT_USED) { // EMPTY or TOMBSTONE -> new insert slot->state = TABLE_SLOT_USED; slot->key = table->key_copy_func(key); if (!slot->key) { - logFatal("No memory for string copy in Table_Set()."); + logFatal("No memory for key copy in Table_Set()."); } table->used++; } @@ } -void *Table_Get(const Table *table, const void *key) { +void *Table_Get(const Table *table, const void *key) { @@ - if (slot->state == TABLE_SLOT_EMPTY) { - return NULL; - } - return slot->value; + if (slot->state != TABLE_SLOT_USED) { + return NULL; + } + return slot->value; }
🧹 Nitpick comments (32)
src/common/iniparser.h (1)
60-61: Allowing ‘:’ in keys — verify scanner and ambiguity.If iniparser.c tokenization uses a mirrored KEY_CHAR set, ensure it’s updated too. Also confirm no ambiguity with assignment (still ‘=’) and that section names with ‘:’ parse as intended. Recommend adding tests for keys like scope:name and sections [lang:cpp].
CMakeLists.txt (1)
57-62: Symlink to data may fail on Windows; add portable fallback.create_symlink can require admin/dev-mode on Windows and fail silently here. Provide a fallback copy to keep tests/data available cross‑platform.
-execute_process( - COMMAND ${CMAKE_COMMAND} -E create_symlink - ${CMAKE_SOURCE_DIR}/data - ${CMAKE_BINARY_DIR}/data -) +execute_process( + COMMAND ${CMAKE_COMMAND} -E create_symlink + ${CMAKE_SOURCE_DIR}/data + ${CMAKE_BINARY_DIR}/data + RESULT_VARIABLE _symlink_result +) +if(NOT _symlink_result EQUAL 0) + message(WARNING "Symlink to data/ failed; falling back to copy.") + file(COPY ${CMAKE_SOURCE_DIR}/data DESTINATION ${CMAKE_BINARY_DIR}) +endif()src/io/file.h (2)
19-19: limits.h error in tool output — keep include, but add portable fallback if needed.limits.h is the right header for PATH_MAX on POSIX; the clang error likely indicates an environment issue. If you want a portable fallback, gate it:
-#include <limits.h> // PATH_MAX +#if defined(_WIN32) + #include <windows.h> + #ifndef PATH_MAX + #define PATH_MAX MAX_PATH + #endif +#else + #include <limits.h> // PATH_MAX +#endifPlease confirm CI toolchain has standard headers available (POSIX or MSVC).
56-64: Clarify search order in docs and error signaling.Document actual precedence implemented by find_project_file() and consider returning a richer status (e.g., errno or enum) for better diagnostics when lookup fails.
src/widgets/components/bottombar.c (1)
71-71: Initialize from parent safely and reuse on_resize for consistency.
Guard parent and drive initial geometry via your resize handler to keep logic in one place.void BottomBar_Init(BottomBar *self, Widget *parent) { Widget_Init(AS_WIDGET(self), parent, &bottombar_ops); - self->base.x = 0; - self->base.y = 0; - self->base.width = parent->width; - self->base.height = 1; + self->base.x = 0; + self->base.y = 0; + self->base.width = parent ? parent->width : 0; + self->base.height = 1; + if (parent) { + bottombar_handle_resize(AS_WIDGET(self), parent->width, parent->height); + }Ensure parent is always non-NULL here; otherwise this guard is required.
tests/test_table.c (1)
118-129: Avoid integer-to-pointer casts in tests; use real addresses.
Casting small integers to pointers is implementation-defined. Use distinct object addresses to avoid UB.void test_ptr_table(void) { Table *table = Table_CreatePtr(); - - Table_Set(table, (void*)1, strdup("Foobar"), free); - Table_Set(table, (void*)2, strdup("Blub"), free); - - TEST_CHECK(strcmp(Table_Get(table, (void*)1), "Foobar") == 0); - TEST_CHECK(strcmp(Table_Get(table, (void*)2), "Blub") == 0); + int k1, k2; + void *key1 = (void*)&k1; + void *key2 = (void*)&k2; + Table_Set(table, key1, strdup("Foobar"), free); + Table_Set(table, key2, strdup("Blub"), free); + + TEST_CHECK(strcmp(Table_Get(table, key1), "Foobar") == 0); + TEST_CHECK(strcmp(Table_Get(table, key2), "Blub") == 0); Table_Destroy(table); }Run this on both 32- and 64-bit to ensure no pointer-size assumptions leak into the test.
Also applies to: 136-136
data/syntax/ini.ini (4)
8-8: Duplicate child entry in root.
commentappears twice; dedupe to avoid redundant scans and potential double-links.-[block:root] -child_blocks = comment, section, comment, assignment +[block:root] +child_blocks = comment, section, assignment
26-30: Tighten assignment matcher.Anchor at BOL and use explicit whitespace to reduce false positives mid‑line.
-start = [ \t]*[.\-_:a-zA-Z0-9]+[ \t]*=[ \t]* +start = "^[ \t]*[.\-_:a-zA-Z0-9]+[ \t]*=[ \t]*" end = $ color=82 child_blocks= number, string, bare_string, comment
35-40: Quote handling in string block: prefer explicit anchors.Be explicit and consistent with quoting; ensure the end includes EOL fallback.
-start = "\"" -end = "\"|$" +start = "\"" +end = "\"|$"If the parser does not process backslash escapes inside quotes, switch to single quotes for the whole value and use raw double quote inside:
start = '"'andend = '"|$'. Please confirm parser semantics.
41-46: Constrain bare_string to avoid eating comments/quotes.Using
.with end$will swallow everything; since you alreadyends_on = comment, also exclude; # "to reduce over-match.-start = "." -end = $ +start = "[^;#\"]+" +end = "$" color = 67 ends_on = commentsrc/common/stack.h (1)
35-40: API surface looks consistent with implementation.Constructors/copy helpers align with stack.c. Consider moving
STACK_GROW_FACTORto.cif not part of the public contract.data/syntax/md.ini (1)
9-14: Remove stray comment under title1.The comment about INI comments is unrelated/noise here.
[block:title1] -# comments start with ";" or "#" start = "^# (.*)$" color = 33 child_blocks =src/io/file.c (1)
95-108: Sanity‑checkrel_pathto reduce traversal risk.If
rel_pathcan be user‑provided, normalize or reject..components to keep lookups within expected roots.Would you like a small helper (rejects absolute paths and
..segments) added tofile.c?tests/test_syntax_definition.c (1)
176-184: Consider adding cases for duplicatechild_blocksandends_onorder.Add tests to confirm duplicates are ignored (or explicitly allowed) and that
ends_onorder is preserved.I can draft two minimal tests mirroring your existing style.
src/common/config.c (1)
42-50: Init path looks good; minor null‑guard suggestion.If
argv0can be NULL in tests, consider defaultingexe_pathto empty string.src/common/buffer.c (1)
47-55: Avoid non‑standard void arithmetic.*Cast to
char *for portable pointer math.- memcpy(buffer->buffer + new_pos * buffer->element_size, element, buffer->element_size); + memcpy((char*)buffer->buffer + new_pos * buffer->element_size, element, buffer->element_size);- void *source = buffer->buffer + buffer->cursor * buffer->element_size; + void *source = (char*)buffer->buffer + buffer->cursor * buffer->element_size;- memcpy(out_element, buffer->buffer + peek_pos * buffer->element_size, buffer->element_size); + memcpy(out_element, (char*)buffer->buffer + peek_pos * buffer->element_size, buffer->element_size);Also applies to: 64-66, 98-99
src/syntax/loader.h (1)
22-41: Export Init prototype and fix documentation typosThe implementation provides SyntaxHighlightingLoaderError_Init but the header doesn’t declare it. Also fix typos in the doc comment.
typedef struct { SyntaxHighlightingLoaderErrorCode code; SyntaxDefinitionError def_error; ParsingError parsing_error; } SyntaxHighlightingLoaderError; +void SyntaxHighlightingLoaderError_Init(SyntaxHighlightingLoaderError *error); void SyntaxHighlightingLoaderError_Deinit(SyntaxHighlightingLoaderError *error); @@ - * Load the syntax definition for `format` from data/sytnax/<format>.ini + * Load the syntax definition for `format` from data/syntax/<format>.ini @@ - * NULL there is definition file or if an error occured. + * NULL if there is no definition file or if an error occurred. @@ - * Note theat `format` needs to be exactly the same as the filename. + * Note that `format` needs to be exactly the same as the filename.src/syntax/loader.c (1)
31-38: Optional: sanitize format to prevent path traversalformat is interpolated into a relative path. Consider restricting to [A-Za-z0-9_-] to avoid “../” or separators.
// 1. load the ini file char filename[PATH_MAX]; + for (const char *p = format; *p; ++p) { + unsigned char c = (unsigned char)*p; + if (!(c == '_' || c == '-' || (c >= '0' && c <= '9') || + (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z'))) { + error->code = SYNTAX_LOADER_NO_FILENAME; + return NULL; + } + } snprintf(filename, PATH_MAX, "data/syntax/%s.ini", format);If you prefer allowing dots (e.g., “c++”), adjust the predicate accordingly and add tests in tests/test_syntax_loader.c.
src/common/stack.c (1)
142-148: Make Stack_IsEmpty/Stack_Size null-safe.Small guard avoids UB at call sites.
bool Stack_IsEmpty(const Stack *stack) { - return stack->size == 0; + return !stack || stack->size == 0; } size_t Stack_Size(const Stack *stack) { - return stack->size; + return stack ? stack->size : 0; }src/main.c (1)
47-50: Update help text to reflect supported options.-static void print_help(const char *program_name) { - fprintf(stderr, "Usage:\n %s <filename>\n", program_name); +static void print_help(const char *program_name) { + fprintf(stderr, "Usage:\n %s [-s <syntax>] <filename>\n %s -h\n", program_name, program_name); exit(0); }tests/test_syntax_highlighting.c (1)
58-59: Minor test hygiene: avoid String_Format for literals; fix message text.
- Prefer String_FromCStr for non-formatted literals to skip a vsnprintf pass.
- Clarify the assertion message.
- String str = String_Format(testcase.str); + String str = String_FromCStr(testcase.str, strlen(testcase.str)); @@ - 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 be non-empty.");Also applies to: 91-93
src/common/table.c (2)
30-41: Destructor vs key free order.If a value destructor needs the key, freeing the key first is risky. Prefer destroying
valuebefore freeing the key. Low likelihood, but safer ordering.- if (slot->key && free_key_func) { - free_key_func(slot->key); - } - if (slot->destructor) { + if (slot->destructor) { slot->destructor(slot->value); } + if (slot->key && free_key_func) { + free_key_func(slot->key); + }
169-176: Initialize function pointers to NULL in Table_Init.Defensive: prevents accidental use of uninitialized callbacks when
Table_Initis used directly.void Table_Init(Table *table) { @@ table->slots = NULL; table->capacity = 0; table->used = 0; + table->hash_func = NULL; + table->key_cmp_func = NULL; + table->key_copy_func = NULL; + table->key_free_func = NULL; }src/syntax/definition.h (1)
151-161: Doc typos and clarity.“strcut”, “occures”, and pluralization nits. Consider fixing to reduce confusion in public API docs.
src/syntax/textlayoutbindings.c (4)
26-45: Make open_blocks param const; stop when state repeats (good).The function doesn’t mutate
open_blocks; takeconst Stack *to document intent. Logic to stop when state matches is solid.-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) { + const Stack *open_blocks_begin = open_blocks;
56-71: Avoid recursion for previous-line priming.Recursive call on long documents can blow the stack. Convert to an iterative walk up to the nearest highlighted ancestor line.
- if (prev_line) { - // check if highlighting for the previous line is already calculated - 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_UpdateLine(binding, prev_line, last_line); - return; - } - } + if (prev_line) { + const Line *seek = prev_line; + const Stack *found = NULL; + while (seek) { + SyntaxHighlightingString *shs_prev = Table_Get(binding->sh->strings, &seek->text); + if (shs_prev) { found = &shs_prev->open_blocks_at_end; break; } + seek = seek->prev; + } + if (found) { + open_blocks = (Stack*)found; // cast to reuse variable; function only reads it + } else { + // prime from the begin of file + seek = TextBuffer_GetFirstLine(binding->tl->tb); + update_following_lines(binding, seek, prev_line, NULL); + SyntaxHighlightingString *shs_prev = Table_Get(binding->sh->strings, &prev_line->text); + open_blocks = shs_prev ? &shs_prev->open_blocks_at_end : NULL; + } + }
72-77: Merging the gap on update disables gap-buffer benefits.This forces compaction on every current-line update. If acceptable for now, gate it behind a config or limit to visible updates; otherwise, compute highlighting over a logical view of current line without merging.
57-58: Remove dead code and commented-out frees.
open_blocksis only used as a const input; the commentedStack_Destroyis irrelevant and misleading.Also applies to: 81-83
src/common/table.h (1)
71-90: API surface looks good for generic keys. Minor doc polish.Small nits in comments (“occure”, “ligit”, “with custom keys” spacing). Consider clarifying comparator contract: “key_cmp_func(a,b) must return 0 when equal, non‑zero otherwise.”
src/syntax/highlighting.c (2)
71-79: Off‑by‑one in capacity check.
tags_count + 1 >= tags_capacityreallocates one step too early. Check>=against current count instead.- if (shs->tags_count + 1 >= shs->tags_capacity) { + if (shs->tags_count >= shs->tags_capacity) { increase_tags_capacity(shs); }
89-100: Index type for reverse scan can overflow int.Use
size_tfor indices and a reverse loop that avoids signed casts.- for (int i=(int)shs->tags_count-1; i>=0; i--) { - if (shs->tags[i].byte_offset == offset) { - return &shs->tags[i]; - } - } + for (size_t i = shs->tags_count; i > 0; i--) { + const size_t idx = i - 1; + if (shs->tags[idx].byte_offset == offset) { + return &shs->tags[idx]; + } + }src/syntax/highlighting.h (1)
50-51: Consider whetherSyntaxHighlightingTag_Deinitis necessary.The implementation shows that
Deinitsimply callsInit, which only resets fields to NULL/0. Since the struct doesn't own any resources (as documented on line 42), theDeinitfunction provides no cleanup value. Consider removing it or adding a comment explaining why resetting fields is important for this type.
📜 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 (37)
.coderabbit.yaml(1 hunks)CMakeLists.txt(2 hunks)data/syntax/ini.ini(1 hunks)data/syntax/md.ini(1 hunks)src/common/buffer.c(2 hunks)src/common/buffer.h(3 hunks)src/common/config.c(4 hunks)src/common/config.h(1 hunks)src/common/iniparser.c(1 hunks)src/common/iniparser.h(1 hunks)src/common/stack.c(3 hunks)src/common/stack.h(1 hunks)src/common/string.c(2 hunks)src/common/string.h(1 hunks)src/common/table.c(14 hunks)src/common/table.h(4 hunks)src/io/file.c(3 hunks)src/io/file.h(3 hunks)src/main.c(7 hunks)src/syntax/definition.c(1 hunks)src/syntax/definition.h(1 hunks)src/syntax/highlighting.c(1 hunks)src/syntax/highlighting.h(1 hunks)src/syntax/loader.c(1 hunks)src/syntax/loader.h(1 hunks)src/syntax/textlayoutbindings.c(1 hunks)src/syntax/textlayoutbindings.h(1 hunks)src/widgets/components/bottombar.c(1 hunks)src/widgets/components/editor.c(6 hunks)src/widgets/components/editor.h(2 hunks)tests/CMakeLists.txt(1 hunks)tests/test_string.c(1 hunks)tests/test_syntax_definition.c(1 hunks)tests/test_syntax_highlighting.c(1 hunks)tests/test_syntax_loader.c(1 hunks)tests/test_syntax_textlayoutbindings.c(1 hunks)tests/test_table.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-25T12:50:44.423Z
Learnt from: defname
PR: defname/clieditor#88
File: src/io/file.h:19-19
Timestamp: 2025-10-25T12:50:44.423Z
Learning: In src/io/file.h, the inclusion of <limits.h> to expose PATH_MAX is intentional so that users of the File module have access to PATH_MAX for working with file paths.
Applied to files:
src/io/file.h
🧬 Code graph analysis (22)
tests/test_syntax_definition.c (4)
src/common/table.c (2)
Table_Create(194-200)Table_Destroy(228-234)src/common/typedtable.c (2)
TypedTable_SetStringCopy(54-60)TypedTable_SetNumber(39-44)src/syntax/definition.c (5)
SyntaxBlockDef_FromTable(73-132)SyntaxBlockDef_Destroy(56-70)SyntaxDefinitionError_Deinit(36-38)SyntaxDefinition_FromTable(329-375)SyntaxDefinition_Destroy(377-391)src/common/iniparser.c (4)
IniParser_Init(362-367)IniParser_SetText(389-391)IniParser_Parse(401-421)IniParser_Deinit(369-372)
src/common/stack.h (1)
src/common/stack.c (6)
Stack_Create(56-63)Stack_Destroy(65-71)Stack_Copy(73-80)Stack_CopyTo(82-103)Stack_IsEmpty(142-144)Stack_Size(146-148)
src/common/buffer.h (1)
src/common/buffer.c (6)
Buffer_Clear(42-45)Buffer_IsEmpty(73-75)Buffer_Size(77-79)Buffer_Capacity(81-83)Buffer_HasSpace(85-87)Buffer_Peek(89-100)
src/syntax/definition.c (4)
src/common/string.c (9)
String_AsCStr(331-333)String_Take(318-329)String_Deinit(264-281)String_Format(404-431)String_FromView(433-435)String_Trim(591-612)String_Length(301-303)String_FromCStr(335-351)String_Split(614-669)src/common/typedtable.c (3)
TypedTable_GetString(100-109)TypedTable_GetNumber(89-98)TypedTable_GetTable(122-131)src/common/table.c (5)
Table_GetUsage(336-341)Table_Set(237-263)Table_Get(265-282)Table_Create(194-200)Table_Destroy(228-234)src/common/tableiterator.c (2)
TableIterator_Begin(3-9)TableIterator_Next(11-24)
src/syntax/highlighting.c (3)
src/common/stack.c (7)
Stack_Init(39-44)Stack_Deinit(46-54)Stack_Clear(150-155)Stack_CopyTo(82-103)Stack_Push(105-114)Stack_Peek(123-128)Stack_Pop(116-121)src/common/table.c (4)
Table_CreatePtr(202-208)Table_Destroy(228-234)Table_Get(265-282)Table_Set(237-263)src/syntax/definition.c (1)
SyntaxDefinition_Destroy(377-391)
src/syntax/loader.c (5)
src/syntax/definition.c (2)
SyntaxDefinitionError_Deinit(36-38)SyntaxDefinition_FromTable(329-375)src/io/file.c (3)
File_OpenProjectFile(122-128)File_Read(169-193)File_Close(130-141)src/common/iniparser.c (5)
IniParser_Init(362-367)IniParser_SetText(389-391)IniParser_Parse(401-421)IniParser_GetError(397-399)IniParser_Deinit(369-372)src/common/table.c (1)
Table_Destroy(228-234)src/syntax/highlighting.c (1)
SyntaxHighlighting_Create(124-131)
tests/test_syntax_loader.c (3)
src/common/config.c (3)
Config_GetExePath(93-95)Config_Init(42-52)Config_Deinit(54-62)src/syntax/loader.c (2)
SyntaxHighlighting_LoadFromFile(20-76)SyntaxHighlightingLoaderError_Deinit(12-17)src/syntax/highlighting.c (1)
SyntaxHighlighting_Destroy(133-139)
src/io/file.h (1)
src/io/file.c (3)
File_Exists(38-43)File_OpenConfig(114-120)File_OpenProjectFile(122-128)
src/syntax/definition.h (1)
src/syntax/definition.c (6)
SyntaxDefinitionError_Deinit(36-38)SyntaxBlockDef_Create(41-53)SyntaxBlockDef_Destroy(56-70)SyntaxBlockDef_FromTable(73-132)SyntaxDefinition_FromTable(329-375)SyntaxDefinition_Destroy(377-391)
src/common/config.h (1)
src/common/config.c (5)
Config_GetExePath(93-95)Config_SetFilename(97-104)Config_GetFilename(106-108)Config_SetSyntax(111-114)Config_GetSyntax(116-118)
src/syntax/loader.h (1)
src/syntax/loader.c (2)
SyntaxHighlightingLoaderError_Deinit(12-17)SyntaxHighlighting_LoadFromFile(20-76)
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)
src/io/file.c (1)
src/common/config.c (1)
Config_GetExePath(93-95)
tests/test_syntax_highlighting.c (6)
src/common/iniparser.c (4)
IniParser_Init(362-367)IniParser_SetText(389-391)IniParser_Parse(401-421)IniParser_Deinit(369-372)src/syntax/definition.c (1)
SyntaxDefinition_FromTable(329-375)src/common/table.c (4)
Table_Destroy(228-234)Table_Create(194-200)Table_Set(237-263)Table_Get(265-282)src/common/string.c (3)
String_Format(404-431)String_Deinit(264-281)String_FromCStr(335-351)src/syntax/highlighting.c (3)
SyntaxHighlighting_Init(105-108)SyntaxHighlighting_HighlightString(210-323)SyntaxHighlighting_Deinit(110-122)src/common/stack.c (6)
Stack_Create(56-63)Stack_Push(105-114)Stack_IsEmpty(142-144)Stack_Size(146-148)Stack_Destroy(65-71)Stack_Peek(123-128)
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)
tests/test_table.c (1)
src/common/table.c (4)
Table_CreatePtr(202-208)Table_Set(237-263)Table_Get(265-282)Table_Destroy(228-234)
src/syntax/highlighting.h (1)
src/syntax/highlighting.c (12)
SyntaxHighlightingTag_Init(7-11)SyntaxHighlightingTag_Deinit(13-15)SyntaxHighlightingString_Create(20-39)SyntaxHighlightingString_Destroy(41-53)SyntaxHighlightingString_AddTag(71-79)SyntaxHighlightingString_Clear(81-87)SyntaxHighlightingString_GetTag(89-100)SyntaxHighlighting_Init(105-108)SyntaxHighlighting_Deinit(110-122)SyntaxHighlighting_Create(124-131)SyntaxHighlighting_Destroy(133-139)SyntaxHighlighting_HighlightString(210-323)
src/common/stack.c (2)
src/common/table.c (1)
increase_capacity(121-167)src/display/widget.c (1)
increase_capacity(86-106)
tests/test_syntax_textlayoutbindings.c (9)
src/document/textbuffer.c (4)
TextBuffer_Init(29-34)TextBuffer_InsertLineAtBottom(90-94)TextBuffer_GetFirstLine(105-111)TextBuffer_Deinit(36-48)src/common/string.c (2)
String_FromCStr(335-351)String_Take(318-329)src/document/line.c (1)
Line_Create(21-31)src/common/iniparser.c (4)
IniParser_Init(362-367)IniParser_SetText(389-391)IniParser_Parse(401-421)IniParser_Deinit(369-372)src/syntax/definition.c (1)
SyntaxDefinition_FromTable(329-375)src/common/table.c (2)
Table_Destroy(228-234)Table_Get(265-282)src/document/textlayout.c (2)
TextLayout_Init(128-140)TextLayout_Deinit(142-155)src/syntax/highlighting.c (2)
SyntaxHighlighting_Init(105-108)SyntaxHighlighting_Deinit(110-122)src/syntax/textlayoutbindings.c (3)
SyntaxHighlightingBinding_Init(3-7)SyntaxHighlightingBinding_Deinit(9-12)SyntaxHighlightingBinding_UpdateLine(47-83)
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/textbuffer.c (3)
TextBuffer_MergeGap(60-77)TextBuffer_GetLastLine(113-119)TextBuffer_GetFirstLine(105-111)src/document/textlayout.c (1)
TextLayout_GetVisualLine(389-401)
src/common/table.h (1)
src/common/table.c (13)
TableSlot_Init(23-28)TableSlot_Deinit(30-41)Table_Init(169-176)Table_Deinit(178-192)Table_Create(194-200)Table_CreatePtr(202-208)Table_CreateCustom(210-226)Table_Set(237-263)Table_Get(265-282)Table_Delete(284-304)Table_Has(306-319)Table_HasOwnership(321-334)Table_GetUsage(336-341)
src/main.c (4)
src/common/config.c (4)
Config_SetSyntax(111-114)Config_SetFilename(97-104)Config_Init(42-52)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/io/file.c (1)
File_OpenConfig(114-120)
🪛 Clang (14.0.6)
src/io/file.h
[error] 19-19: 'limits.h' file not found
(clang-diagnostic-error)
src/syntax/definition.h
[error] 68-68: 'stdint.h' file not found
(clang-diagnostic-error)
src/syntax/textlayoutbindings.h
[error] 4-4: 'stdbool.h' file not found
(clang-diagnostic-error)
src/syntax/highlighting.h
[error] 30-30: 'stddef.h' file not found
(clang-diagnostic-error)
src/common/table.h
[error] 33-33: 'stdint.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (24)
src/common/string.h (1)
181-181: Doc correction looks right; confirm behavior matches.Change clarifies that String_Take deinitializes src. Please confirm implementation actually deinitializes the source to avoid double-free/aliasing surprises.
.coderabbit.yaml (1)
175-176: LGTM — style-only YAML change.Block-style list for base_branches is semantically identical to ['.*'] and valid per schema.
CMakeLists.txt (2)
8-9: Nice DX improvement.CMAKE_EXPORT_COMPILE_COMMANDS=ON helps tooling (clangd, static analyzers).
64-66: CTest enablement — good.enable_testing() placement at root is correct; subdir tests will be discoverable.
src/common/buffer.h (2)
51-55: Clear API is useful.Resetting count and cursor is correct and matches implementation.
78-85: Const-correctness improvements — good.IsEmpty, Size, Capacity, Peek taking const Buffer* aligns with read-only usage.
Also applies to: 96-96
tests/test_string.c (1)
254-254: No functional change; safe to merge.
Extra blank line only. Nothing to do.src/common/iniparser.c (1)
145-145: Allowing ':' in keys looks good; add targeted tests and doc note.
This enables namespaced keys/sections. Please:
- Add tests for keys with colon (both in sections and assignments).
- Add a negative test to ensure "key: value" (colon-as-assignment) still errors as intended.
src/common/string.c (1)
538-540: Good catch: reserve space for the trailing null.
Updating the capacity check to include +1 prevents off-by-one overwrites on append.Consider adding a regression test appending a view that exactly fills capacity to ensure no overflow and correct termination.
src/common/config.h (1)
32-32: Config_GetExePath API LGTM.
Returning const char* is appropriate; document lifetime (valid until Config_Deinit).tests/CMakeLists.txt (1)
24-24: Code change verified and approved.The verification confirms that
enable_testing()is properly invoked in the root CMakeLists.txt at line 65, which validates the use of$<TARGET_FILE:...>generator expressions in the add_test call. The configuration is correct.src/widgets/components/editor.h (1)
25-25: Lifecycle management of sh_binding is properly handled.Verification confirms that
SyntaxHighlightingBinding_Initis called duringEditor_Init(line 459) andSyntaxHighlightingBinding_Deinitis called during Editor destruction (line 39). The by-value embedding is reasonable for a tightly-coupled binding component. Consider a pointer-based approach only if decoupling from the TextLayout header becomes necessary in the future; the current design avoids heap allocation overhead.src/common/stack.h (1)
47-49: Nice const‑correct accessors.
Stack_IsEmpty/Stack_Sizesignatures are appropriate.src/io/file.c (1)
38-43:File_Existsis fine for read paths.R_OK is sensible given current read‑only usage. If you later use it for write discovery, consider
W_OKorF_OK.src/common/buffer.c (1)
42-46:Buffer_Clearis a welcome addition.Matches typical ring buffer semantics.
data/syntax/ini.ini (1)
47-50: Revert suggested change—original escape pattern is correct.The original pattern correctly matches all escape sequences. After INI string parsing (
\\→\),"\\\\[\\\\\\nt'\"]"becomes regex\\[\\nt'"], which matches\followed by any of:\,n,t,',". The suggested change would remove the ability to match\\(escaped backslash), breaking functionality.Likely an incorrect or invalid review comment.
src/main.c (1)
214-217: Ensure binding is fully initialized, not just sh assigned.Confirm EditorView_Create (or Editor) calls SyntaxHighlightingBinding_Init and sets tl; otherwise call it here to avoid partial state.
Would you confirm where SyntaxHighlightingBinding_Init is invoked for editor->editor->sh_binding? If not initialized, we should do:
SyntaxHighlightingBinding_Init(&editor->editor->sh_binding, editor->editor->layout, highlighting);src/syntax/definition.c (1)
41-71: Solid construction, linking, and cleanup paths.Block creation, error propagation, and cleanup look correct; no issues found.
Also applies to: 142-218, 329-375
src/syntax/highlighting.c (2)
195-208: Good: cache reset for all blocks before a run.This avoids cross‑line contamination. Nice.
110-122: Lifecycle ownership is clear and safe.
SyntaxHighlighting_Deinitdestroys table and def;DestroywrapsDeinit. LGTM.Also applies to: 133-139
src/syntax/definition.h (1)
74-85: ****The code is correct.
NO_ERRORinsrc/syntax/definition.c:26is a macro that properly initializes the entire error structure:#define NO_ERROR ((SyntaxDefinitionError){ .code = SYNTAXDEFINITION_NO_ERROR, .message = (String){0} }). It correctly uses the enum constant from the header and compiles without issues. The assignment at line 74 is valid and idiomatic.Likely an incorrect or invalid review comment.
src/syntax/highlighting.h (3)
76-85: Note the performance consideration forSyntaxHighlightingString_GetTag.The implementation uses a linear search through tags (O(n)), which the developer has already flagged with a TODO comment about needing "massive optimization". For strings with many syntax elements, this could become a bottleneck if called frequently during rendering.
30-30: No action needed—static analysis warning is confirmed as a false positive.The build system exists and multiple files in the codebase, including the file under review, successfully include
stddef.h. This confirms the warning stems from tool configuration rather than an actual missing header.
117-132: Fix parameter name inconsistency in documentation.The documentation on line 127 refers to the parameter as
open_blocks_at_begin, but the function signature on line 132 usesopen_blocks. The implementation file usesopen_blocks_at_begin. Standardize the naming for clarity.* @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 open_blocks_at_begin A 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. * * @returns * A reference to the `Stack` containing all open blocks at the end of `text`. */ -const Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *sh, const String *text, const Stack *open_blocks); +const Stack *SyntaxHighlighting_HighlightString(SyntaxHighlighting *sh, const String *text, const Stack *open_blocks_at_begin);Also note: line 126 has a typo "hightlight" → "highlight" and line 127 has "An pointer" → "A pointer".
Likely an incorrect or invalid review comment.
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/table.c (1)
237-263: Update error message to be generic.Line 253 says "No memory for string copy" but this table now supports arbitrary key types, not just strings.
Apply this diff:
- logFatal("No memory for string copy in Table_Set()."); + logFatal("Key copy failed in Table_Set().");
♻️ Duplicate comments (4)
src/common/table.c (1)
54-57: hash_ptr has UB on 32‑bit platforms (shift by 32).This issue was already identified in a previous review. The right-shift by 32 is undefined behavior when
uintptr_tis 32 bits.src/common/config.c (1)
112-119: Own thesyntaxstring (avoid dangling pointer).This issue was previously flagged and remains unresolved. Storing a raw pointer to the caller's string (e.g., from
optarginmain.cline 64) creates a dangling pointer risk. The string should be owned via duplication, and freed inConfig_Deinit().See the previous review comment for a suggested fix.
src/main.c (1)
156-178: Fix String formatting and ensure unconditional error deinit.This issue was previously flagged and remains unresolved:
- Line 170: Passing
error.def_error.message(aString) to"%s"is undefined behavior. UseString_AsCStr(&error.def_error.message).- Line 176:
SyntaxHighlightingLoaderError_Deinitis only called on failure. It must be called unconditionally (also on success) to avoid leaks.Apply this fix:
case SYNTAX_LOADER_DEFINITION_ERROR: - logError("Syntax definition error.\n%s", error.def_error.message); + logError("Syntax definition error.\n%s", String_AsCStr(&error.def_error.message)); break; default: logError("Could not load Syntaxdefinition (Code: %d)", error.code); break; } - SyntaxHighlightingLoaderError_Deinit(&error); highlighting = NULL; } +SyntaxHighlightingLoaderError_Deinit(&error);src/syntax/textlayoutbindings.c (1)
99-113: Add NULL guard forshlikeUpdatedoes.This issue was previously flagged. Line 100 lacks a
binding->shcheck thatUpdateincludes (line 86). WhileUpdateLineguards against NULLsh, failing fast here improves robustness and consistency.Apply this fix:
void SyntaxHighlightingBinding_UpdateAll(SyntaxHighlightingBinding *binding, bool force) { - if (!binding || !binding->tl || !binding->tl->tb) { + if (!binding || !binding->sh || !binding->tl || !binding->tl->tb) { return; } if (!force && !binding->need_full_update) {
🧹 Nitpick comments (4)
src/common/table.c (1)
194-200: Consider wrapper functions for better type safety.The function pointer casts bypass the type system—
hash_stringexpectsconst char*but the callback signature isconst void*. While this is a common C pattern for generic containers, it relies entirely on runtime discipline. If a caller passes non-string keys to a string table, it will crash or corrupt memory.To improve safety, consider using thin wrapper functions:
static uint32_t hash_string_wrapper(const void *p) { return hash_string((const char *)p); } static int strcmp_wrapper(const void *a, const void *b) { return strcmp((const char *)a, (const char *)b); } static void *strdup_wrapper(const void *p) { return strdup((const char *)p); } Table *Table_Create() { return Table_CreateCustom( hash_string_wrapper, strcmp_wrapper, strdup_wrapper, free); }This makes the type conversions explicit and easier to audit.
src/syntax/textlayoutbindings.c (2)
26-45: Consider removing or clarifying the commented-out code.Line 39 contains
//Stack_Destroy(open_blocks_end);which is correctly not called sinceopen_blocks_endpoints to the internal stack withinshs. Consider removing this commented line or adding a clarifying comment explaining why destruction is not needed.const Stack *open_blocks_end = SyntaxHighlighting_HighlightString(binding->sh, &line->text, open_blocks_begin); open_blocks_begin = open_blocks_end; - //Stack_Destroy(open_blocks_end); + // Note: open_blocks_end points to internal shs->open_blocks_at_end, not owned by us
72-76: Gap merging approach acknowledged as suboptimal.The forced gap merge (line 75) with const-cast is acknowledged as "super dirty" by the inline comments. While this ensures correctness for current highlighting logic (including pointer arithmetic in
editor.cline 115), consider refactoring to a gap-aware solution that doesn't require merging, preserving the gap buffer's performance benefits.Potential approaches:
- Implement gap-aware byte offset calculation that doesn't require pointer arithmetic.
- Highlight on a merged copy rather than modifying the text buffer.
src/syntax/highlighting.c (1)
90-101: Acknowledged optimization opportunity.The linear search (O(n)) for tag lookup is flagged by the TODO comment (line 91). Consider implementing binary search or an index structure if profiling shows this is a bottleneck.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
data/syntax/ini.ini(1 hunks)data/syntax/md.ini(1 hunks)src/common/buffer.c(2 hunks)src/common/config.c(5 hunks)src/common/table.c(14 hunks)src/main.c(6 hunks)src/syntax/highlighting.c(1 hunks)src/syntax/textlayoutbindings.c(1 hunks)src/widgets/components/editor.c(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- data/syntax/md.ini
- data/syntax/ini.ini
🧰 Additional context used
🧬 Code graph analysis (5)
src/syntax/highlighting.c (3)
src/common/stack.c (7)
Stack_Init(39-44)Stack_Deinit(46-54)Stack_Clear(150-155)Stack_CopyTo(82-103)Stack_Push(105-114)Stack_Peek(123-128)Stack_Pop(116-121)src/common/table.c (4)
Table_CreatePtr(202-208)Table_Destroy(228-234)Table_Get(265-282)Table_Set(237-263)src/syntax/definition.c (1)
SyntaxDefinition_Destroy(377-391)
src/main.c (4)
src/common/config.c (4)
Config_SetSyntax(112-115)Config_SetFilename(98-105)Config_Init(42-52)Config_GetSyntax(117-119)src/syntax/highlighting.c (1)
SyntaxHighlighting_Destroy(134-140)src/syntax/loader.c (2)
SyntaxHighlighting_LoadFromFile(20-76)SyntaxHighlightingLoaderError_Deinit(12-17)src/io/file.c (1)
File_OpenConfig(114-120)
src/syntax/textlayoutbindings.c (4)
src/common/table.c (1)
Table_Get(265-282)src/syntax/highlighting.c (1)
SyntaxHighlighting_HighlightString(211-324)src/document/textbuffer.c (3)
TextBuffer_MergeGap(60-77)TextBuffer_GetLastLine(113-119)TextBuffer_GetFirstLine(105-111)src/document/textlayout.c (1)
TextLayout_GetVisualLine(389-401)
src/widgets/components/editor.c (7)
src/syntax/textlayoutbindings.c (4)
SyntaxHighlightingBinding_Deinit(9-12)SyntaxHighlightingBinding_Update(85-97)SyntaxHighlightingBinding_UpdateAll(99-113)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(90-101)src/common/utf8_helper.c (1)
utf8_to_codepoint(57-96)
src/common/config.c (1)
src/common/iniparser.c (1)
IniParser_Deinit(369-372)
🔇 Additional comments (34)
src/common/buffer.c (3)
42-45: LGTM! Clean buffer reset implementation.The function correctly resets the buffer to an empty state by zeroing the count and cursor. The underlying memory is preserved, which is appropriate for reuse.
73-89: LGTM! Excellent const-correctness improvements.The addition of
constqualifiers toBuffer_IsEmpty,Buffer_Size,Buffer_Capacity, andBuffer_Peekappropriately reflects that these operations are read-only. This strengthens the API contract and enables usage with const buffers.
85-87: LGTM! The off-by-one issue has been correctly fixed.The current implementation correctly uses
buffer->count < buffer->capacity, which is consistent withBuffer_Enqueue's capacity check on line 48. This allows the buffer to utilize its full capacity rather than stopping atcapacity - 1. The issue raised in the previous review has been properly addressed.src/common/table.c (9)
30-41: LGTM!The callback-based key deallocation is correctly implemented with proper NULL checks for both the key and the free function.
59-61: LGTM!The pointer comparison correctly implements the equality-check semantics required by the table (returns 0 when equal, non-zero otherwise).
67-98: LGTM!The generic key implementation correctly delegates to the callback functions for hashing and comparison.
121-167: LGTM!The capacity expansion correctly preserves the callback functions and uses them for key cleanup during rehashing.
178-192: LGTM!Table cleanup correctly delegates key deallocation to the configured callback.
202-208: LGTM!Pointer-key table configuration is correct, with NULL for the free function since pointer keys are not owned.
210-226: LGTM!The custom constructor provides the necessary flexibility for arbitrary key types while maintaining a clean API.
265-334: LGTM!The public API correctly handles generic keys and uses the configured callbacks appropriately.
336-341: LGTM!The new usage query function is straightforward and correctly handles NULL input.
src/common/config.c (2)
42-52: LGTM!The updated signature correctly initializes
exe_pathfromargv[0], which remains valid for the program's lifetime. The syntax field is properly initialized to NULL.
98-105: LGTM!Proper bounds checking with
PATH_MAX - 1and explicit null-termination ensure safe string handling.src/widgets/components/editor.c (5)
36-43: LGTM!Proper cleanup sequence: syntax highlighting binding is deinitialized before text selection and layout teardown.
107-122: Verify gap-merging guarantee across the highlighting pipeline.The pointer arithmetic
byte_offset = ch - line->src->text.bytes(line 115) assumeschpoints withinline->src->text.bytes. The inline comments acknowledge this fragility: "This will not always work if is_gap_line and the gap is not merged!!" WhileTextBuffer_MergeGapis called intextlayoutbindings.c(line 75), this creates tight coupling and risks undefined behavior if the gap-merging contract is violated in future changes.Consider one of the following to reduce fragility:
- Add a runtime assertion confirming the gap is merged before calculating
byte_offset.- Refactor to avoid pointer arithmetic by using a gap-aware index calculation helper.
347-356: LGTM!Triggering
SyntaxHighlightingBinding_Updateafter successful input handling ensures highlighting stays synchronized with text changes.
379-394: LGTM!Calling
SyntaxHighlightingBinding_UpdateAllwithforce=falseensures highlighting is initialized once, respecting the internalneed_full_updateflag. Placement after cursor correction is appropriate.
432-459: LGTM!Initializing the binding with
NULLhighlighting is appropriate here, as the actualSyntaxHighlightinginstance is assigned later inmain.c(line 211) after loading the syntax definition.src/main.c (4)
139-141: LGTM!Correctly passes
argv[0]to the updatedConfig_Initsignature.
197-205: LGTM!Using
File_OpenConfigsimplifies config file access and aligns with the new helper API.
209-212: LGTM!Assigning the loaded highlighting to
editor->editor->sh_binding.shcompletes the binding initialization.
119-130: LGTM!
SyntaxHighlighting_Destroyinfinish()ensures proper cleanup. The function handles NULL safely.src/syntax/textlayoutbindings.c (2)
3-12: LGTM!Straightforward initialization and deinitialization of the binding, storing references and managing the update flag.
85-97: LGTM!Comprehensive NULL guards and correct determination of the visible line range for incremental updates.
src/syntax/highlighting.c (9)
7-16: LGTM!Simple tag lifecycle management with future extensibility in mind.
21-54: LGTM!Proper allocation, initialization, and cleanup of
SyntaxHighlightingStringwith appropriate error handling.
56-70: LGTM!Standard capacity growth pattern with appropriate error handling.
72-88: LGTM!
AddTagandClearcorrectly manage tags and stack state. Efficient reuse of allocated capacity.
106-140: LGTM!Clean lifecycle management with proper ownership transfer of the syntax definition.
142-159: LGTM!Clever regex match caching avoids redundant regex execution by reusing matches when the search offset moves forward. The offset arithmetic correctly translates cached absolute positions to new relative positions.
161-193: LGTM!Helper functions correctly find the earliest matching block using cached regex execution.
196-209: LGTM!Proper cache initialization before highlighting ensures clean state.
211-324: LGTM!The core highlighting algorithm correctly implements a stack-based state machine that:
- Identifies the earliest match among child start, ends_on, and current block end
- Maintains open-block state via stack operations
- Generates tags at precise byte offsets
- Returns open-block state for cross-line propagation
The design assumes well-formed syntax definitions where the root block's end pattern never matches (line 321-322 comment). This is an appropriate contract.
| static void *cpy_ptr(const void *p) { | ||
| return (void*)p; | ||
| } |
There was a problem hiding this comment.
Minor const-correctness issue: casting away const.
The cast (void*)p discards the const qualifier. While harmless here since pointer keys aren't modified, consider returning (void *)(uintptr_t)p to make the intent clearer.
🤖 Prompt for AI Agents
In src/common/table.c around lines 63 to 65, the helper cpy_ptr currently casts
away const via (void*)p; change it to return (void*)(uintptr_t)p to preserve
intent without discarding const, and ensure uintptr_t is available by adding
#include <stdint.h> at the top of the file if not already present; no other
logic changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/common/table.c (2)
54-57: UB on 32-bit platforms remains unfixed.This issue was previously flagged:
(x >> 32)is undefined whenuintptr_tis 32 bits. The shift count must not equal or exceed the bit-width of the operand.
63-65: Const-correctness issue remains unaddressed.This was previously noted:
(void*)pdiscards the const qualifier. Use(void*)(uintptr_t)pto make the intent clearer.
🧹 Nitpick comments (1)
src/common/table.c (1)
210-226: Consider validating mandatory callback parameters.
Table_CreateCustomdoes not check whetherhash_func,key_cmp_func, orkey_copy_funcare non-NULL. If any are NULL, the table will crash when used (e.g., lines 75, 89, 251). Either add validation withlogFatalfor these mandatory parameters, or document the precondition that callers must provide non-NULL callbacks.Apply this diff to add validation:
Table *Table_CreateCustom( uint32_t (*hash_func)(const void *p), int (*key_cmp_func)(const void*, const void *), void *(*key_copy_func)(const void *p), void (*key_free_func)(void *p) ) { + if (!hash_func || !key_cmp_func || !key_copy_func) { + logFatal("hash_func, key_cmp_func, and key_copy_func must be non-NULL in Table_CreateCustom."); + } Table *table = malloc(sizeof(Table)); if (!table) { logFatal("Failed to allocate memory for table."); } Table_Init(table); table->hash_func = hash_func; table->key_cmp_func = key_cmp_func; table->key_copy_func = key_copy_func; table->key_free_func = key_free_func; return table; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/common/table.c(14 hunks)
🔇 Additional comments (5)
src/common/table.c (5)
30-41: LGTM: Clean parameterization of key deallocation.The updated signature correctly delegates key cleanup to the caller-supplied callback, maintaining consistency with the generic-key design.
59-61: LGTM: Comparison semantics are correct.Returns 0 when pointers are equal, non-zero otherwise, matching the expected comparison function contract.
194-208: LGTM: Constructor refactoring is well-structured.Both
Table_CreateandTable_CreatePtrcorrectly delegate toTable_CreateCustomwith appropriate callbacks. The function pointer casts inTable_Create(lines 196-199) are safe because runtime arguments will match the original string-based types.
121-167: LGTM: Resize correctly preserves callback context.
increase_capacityproperly propagates all four callbacks to the resized table (lines 126-131) and correctly frees old keys viakey_free_func(lines 151-153) before deallocating slots. The ownership transfer inshallow_copy_table(line 117) is handled correctly.
237-341: LGTM: Public API correctly adopts generic-key semantics.All public functions (
Table_Set,Table_Get,Table_Delete,Table_Has,Table_HasOwnership) now acceptconst void *keyand delegate key operations to the stored callbacks. The newTable_GetUsageaccessor is straightforward and correct.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
README.md (3)
59-59: Fix hyphenation in compound adjective."UTF-8 aware" should be hyphenated as "UTF-8-aware" when used as a compound adjective before a noun.
-The editor uses a custom, UTF-8 aware string library designed for a balance of performance and memory efficiency. +The editor uses a custom, UTF-8-aware string library designed for a balance of performance and memory efficiency.
102-106: Correct markdown list indentation.The nested list items under "Naming Conventions" use 4-space indentation, but markdown lists should use 2-space indentation for consistency.
- **Naming Conventions**: - - Types and structs are `PascalCase` (e.g., `TextBuffer`). - - Public functions are `Module_Function` (e.g., `TextBuffer_Init`). - - Enums and macros are `ALL_CAPS`. + - Types and structs are `PascalCase` (e.g., `TextBuffer`). + - Public functions are `Module_Function` (e.g., `TextBuffer_Init`). + - Enums and macros are `ALL_CAPS`.
128-128: Format bare URL as markdown link.The GPL-3.0 license URL should be properly formatted as a markdown link rather than a bare URL.
-You may obtain a copy of the license at: https://www.gnu.org/licenses/gpl-3.0.html +You may obtain a copy of the license at: [https://www.gnu.org/licenses/gpl-3.0.html](https://www.gnu.org/licenses/gpl-3.0.html)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~59-~59: Use a hyphen to join words.
Context: ... System The editor uses a custom, UTF-8 aware string library designed for a bala...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
103-103: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
104-104: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
105-105: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
128-128: Bare URL used
(MD034, no-bare-urls)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~59-~59: Use a hyphen to join words.
Context: ... System The editor uses a custom, UTF-8 aware string library designed for a bala...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
103-103: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
103-103: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
104-104: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
104-104: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
105-105: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
105-105: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
128-128: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (1)
README.md (1)
1-128: Comprehensive architectural documentation; well-structured rewrite.The README has been thoroughly rewritten to document the new syntax highlighting subsystem and overall architecture. The structure is clear and logical:
- Features are well-articulated with an emphasis on implementation choices (gap buffer, regex-based highlighting, UTF-8 efficiency).
- Architecture Overview provides good module separation and clarifies the role of the new
syntax/module.- Core Concepts explain key design decisions (String System caching, Widget System inheritance, Editor Engine layering, Syntax Highlighting declarative definitions).
- Practical sections (How to Read, Coding Style, Future Goals) are detailed and actionable.
- Usage documentation includes the new
-ssyntax flag with clear examples.The documentation aligns well with the PR's objectives and provides a solid foundation for onboarding and future development.
| ## Features | ||
|
|
||
| - **Handwritten UI Toolkit**: A custom-built widget system from scratch, featuring a status bar, main menu, notifications, and an editor widget. | ||
| - **Syntax Highlighting**: A flexible, regex-based highlighting engine. Syntaxes are defined in simple `.ini` files, making it easy to add new languages. |
There was a problem hiding this comment.
Hyphenate compound adjective "UTF-8-aware".
When "UTF-8-aware" precedes a noun, it should be hyphenated to form a compound adjective. This appears in two places: line 12 (Features section) and line 59 (Core Concepts section).
- - **Syntax Highlighting**: A flexible, regex-based highlighting engine. Syntaxes are defined in simple `.ini` files, making it easy to add new languages.
+ - **Syntax Highlighting**: A flexible, regex-based highlighting engine. Syntaxes are defined in simple `.ini` files, making it easy to add new languages.
- - **Efficient UTF-8 Support**: A revised string library that handles UTF-8 characters correctly and efficiently, with optimized indexing for large strings.
+ - **Efficient UTF-8-aware Support**: A revised string library that handles UTF-8 characters correctly and efficiently, with optimized indexing for large strings.And on line 59:
- The editor uses a custom, UTF-8 aware string library designed for a balance of performance and memory efficiency.
+ The editor uses a custom, UTF-8-aware string library designed for a balance of performance and memory efficiency.Also applies to: 59-59
🤖 Prompt for AI Agents
In README.md around lines 12 and 59, the phrase "UTF-8 aware" should be
hyphenated as "UTF-8-aware" when used as a compound adjective preceding a noun;
update both occurrences to "UTF-8-aware" to follow compound adjective
hyphenation rules.
| - **Naming Conventions**: | ||
| - Types and structs are `PascalCase` (e.g., `TextBuffer`). | ||
| - Public functions are `Module_Function` (e.g., `TextBuffer_Init`). | ||
| - Enums and macros are `ALL_CAPS`. |
There was a problem hiding this comment.
Fix markdown list indentation.
The nested list items under "Naming Conventions" are indented with 2 spaces, but markdown expects them to align with their parent list markers (0 indentation). This causes markdownlint to flag inconsistent indentation.
- **Naming Conventions**:
- - Types and structs are `PascalCase` (e.g., `TextBuffer`).
- - Public functions are `Module_Function` (e.g., `TextBuffer_Init`).
- - Enums and macros are `ALL_CAPS`.
+ - Types and structs are `PascalCase` (e.g., `TextBuffer`).
+ - Public functions are `Module_Function` (e.g., `TextBuffer_Init`).
+ - Enums and macros are `ALL_CAPS`.📝 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.
| - **Naming Conventions**: | |
| - Types and structs are `PascalCase` (e.g., `TextBuffer`). | |
| - Public functions are `Module_Function` (e.g., `TextBuffer_Init`). | |
| - Enums and macros are `ALL_CAPS`. | |
| - **Naming Conventions**: | |
| - Types and structs are `PascalCase` (e.g., `TextBuffer`). | |
| - Public functions are `Module_Function` (e.g., `TextBuffer_Init`). | |
| - Enums and macros are `ALL_CAPS`. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
103-103: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
103-103: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
104-104: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
104-104: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
105-105: Inconsistent indentation for list items at the same level
Expected: 0; Actual: 2
(MD005, list-indent)
105-105: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In README.md around lines 102 to 105, the nested list under "Naming Conventions"
is indented with two leading spaces causing markdownlint to flag inconsistent
indentation; remove the extra two spaces so the nested list items align directly
under the parent list marker (i.e., start the lines with the list marker at
column 0), preserving the backtick formatting for examples.
| This project is licensed under the GNU General Public License v3.0 (GPL-3.0). | ||
|
|
||
| https://www.gnu.org/licenses/gpl-3.0.html | ||
| You may obtain a copy of the license at: https://www.gnu.org/licenses/gpl-3.0.html |
There was a problem hiding this comment.
Wrap bare URL in markdown link syntax.
Line 128 contains a bare URL. Per markdownlint (MD034), URLs should be wrapped in markdown link syntax or angle brackets.
- You may obtain a copy of the license at: https://www.gnu.org/licenses/gpl-3.0.html
+ You may obtain a copy of the license at: <https://www.gnu.org/licenses/gpl-3.0.html>📝 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.
| You may obtain a copy of the license at: https://www.gnu.org/licenses/gpl-3.0.html | |
| You may obtain a copy of the license at: <https://www.gnu.org/licenses/gpl-3.0.html> |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
128-128: Bare URL used
(MD034, no-bare-urls)
🤖 Prompt for AI Agents
In README.md around line 128, the license URL is a bare link which violates
markdownlint MD034; replace the bare URL with markdown link syntax by wrapping
the URL in square brackets with descriptive text and the URL in parentheses
(e.g., [GNU GPL v3](https://www.gnu.org/licenses/gpl-3.0.html)) or wrap the bare
URL in angle brackets (<https://...>) so the line becomes a proper markdown
link.
1 participant
Summary by CodeRabbit
New Features
Build & Testing
UI
Documentation