Add table iteration API and improve parsing robustness - #83
Conversation
* add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool
* Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete()
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces robust error handling for syntax definition parsing, adds a table iteration API with public accessor functions, strengthens null-pointer safety in utility functions, extends INI key syntax to permit colons, and supplies comprehensive test coverage for the new features. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant init_definition
participant build_blocks
participant link_children
participant Error Handler
Caller->>init_definition: SyntaxDefinition_FromTable(table, error)
alt Initialization Success
init_definition->>build_blocks: Enumerate block definitions
alt Block Building Success
build_blocks->>link_children: Resolve allowed_blocks relationships
alt Linking Success
link_children-->>Caller: Return SyntaxDefinition*
else Linking Failure
link_children->>Error Handler: Set error (missing/invalid references)
Error Handler-->>Caller: Return NULL
end
else Block Building Failure
build_blocks->>Error Handler: Set error (no start regex, invalid regex, etc.)
Error Handler-->>Caller: Return NULL
end
else Initialization Failure
init_definition->>Error Handler: Set error (no root block, allocation failure)
Error Handler-->>Caller: Return NULL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes span multiple domains (CMake, INI parsing, string/table utilities, syntax definitions, testing) with high heterogeneity. The syntax definition refactor introduces substantial new logic with error handling and a multi-phase pipeline. The table iterator and table enhancements add new public APIs requiring careful contract validation. Diverse file changes and new test coverage add cumulative review complexity. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
coderabbitai
Bot
changed the title
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/syntax/definition.h (1)
4-8: Missing include for bool in a public header.This header uses bool but doesn’t include <stdbool.h>. This can break consumers that don’t include it beforehand.
#include <stdint.h> #include <regex.h> +#include <stdbool.h> #include "common/table.h" #include "common/string.h"src/common/string.c (1)
318-329: Add self-assignment guard in String_Take to avoid undefined behavior.
dst==src currently frees src then dereferences it. Guard it.void String_Take(String *dst, String *src) { - if (!dst || ! src) { + if (!dst || !src) { return; } + if (dst == src) { + return; // no-op self move + } String_Deinit(dst); *dst = String_TakeCStr(src->bytes); src->bytes = NULL; src->bytes_capacity = 0; src->bytes_size = 0; String_Deinit(src); }Optional: preserve multibyte cache and avoid strlen by moving the whole struct.
void String_Take(String *dst, String *src) { - if (!dst || !src) { return; } - if (dst == src) { return; } - String_Deinit(dst); - *dst = String_TakeCStr(src->bytes); - src->bytes = NULL; - src->bytes_capacity = 0; - src->bytes_size = 0; - String_Deinit(src); + if (!dst || !src || dst == src) { return; } + String_Deinit(dst); + *dst = *src; // move all fields (bytes + multibytes) + src->bytes = NULL; src->bytes_capacity = 0; src->bytes_size = 0; + src->multibytes = NULL; src->multibytes_capacity = 0; src->multibytes_size = 0; + src->multibytes_invalid = false; src->char_count = 0; }
🧹 Nitpick comments (14)
src/common/table.h (1)
100-104: Clarify semantics of “usage” (tombstones included); consider naming.Doc says “non-free slots” but doesn’t state tombstones are counted. Suggest clarify and/or consider a more explicit name (e.g., Table_GetOccupiedCount).
/** - * @brief Return the current number of non-free table slots. + * @brief Return the current number of non-free slots (USED + TOMBSTONE). + * Note: Includes tombstones; use this to gauge load factor, not live key count. */ size_t Table_GetUsage(const Table *table);tests/CMakeLists.txt (1)
24-24: Good switch to $<TARGET_FILE:…>.This is the right way to register tests. If tests rely on relative fixtures, set an explicit working dir:
set_tests_properties(${TEST_NAME} PROPERTIES WORKING_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}")src/syntax/definition.h (4)
9-31: Error type/API looks good; document lifecycle.Add a brief note that SyntaxDefinitionError.message must be deinitialized with SyntaxDefinitionError_Deinit, and that passing error=NULL is allowed (or not).
37-47: Ownership semantics for name/children must be clear.Ensure:
- SyntaxBlockDef_FromTable duplicates name (heap) and SyntaxBlockDef_Destroy frees it.
- children array ownership is with SyntaxBlockDef and freed in destroy.
Add short docs to avoid misuse.
51-52: FromTable constructor: note nullability and error contract.Specify whether error can be NULL and whether partially-built objects are cleaned up on failure.
61-62: Expose blocks_count: confirm invariant and ordering.Document whether blocks are topologically ordered and if blocks_count includes root. Minor, but helpful for iterators.
CMakeLists.txt (1)
56-57: Scope CTest to top-level builds to avoid side-effects when used as a subproject.
Wrap enable_testing() so downstreams aren’t forced into CTest unintentionally; optionally add include(CTest).-# CTest-Unterstützung aktivieren -enable_testing() +# CTest-Unterstützung aktivieren (nur im Top‑Level-Projekt) +if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) + include(CTest) + enable_testing() +endif()src/common/string.h (1)
177-183: Doc polish and clarity for ownership transfer.
Clarify wording and fix typos; behavior note (“src will be deinitialized”) is correct.- * @brief Transfer the owenership of src to dest. + * @brief Transfer the ownership of src to dst. * - * Its for situation where dst is already initialized and your want to fill it + * It's for situations where dst is already initialized and you want to fill it * with the content of another string without moving memory around. - * src will be deinitialized! + * src will be deinitialized (ownership moves to dst).tests/test_table.c (1)
108-115: Nice empty-table coverage; consider asserting usage as well.
If Table_GetUsage is part of the public API, add an assertion to lock behavior down.// checks on empty table table = Table_Create(); + // optional: verify usage on a fresh table + // TEST_CHECK(Table_GetUsage(table) == 0); TEST_CHECK(!Table_Get(table, "key")); TEST_CHECK(!Table_Has(table, "key")); TEST_CHECK(!Table_HasOwnership(table, "key"));src/common/tableiterator.c (1)
3-9: Iterator core looks good; consider clarifying the start index sentinel.Logic is correct and null-safe. One small robustness nit: using -1 as the initial index relies on signedness/underflow subtleties when computing
(size_t)(it->index + 1). Consider documenting the intended type ofTableIterator.index(signed) or switch to an explicit sentinel (e.g., SIZE_MAX) with a computedstartvariable to avoid casts.Please confirm
TableIterator.indexis a signed type incommon/tableiterator.h. If it’s unsigned, prefer a SIZE_MAX sentinel and compute:
size_t start = (it->index == SIZE_MAX) ? 0 : (it->index + 1);Also applies to: 11-24
tests/test_tableiterator.c (1)
12-20: Avoid asserting internal slot state in iterator tests.
TEST_CHECK(it.current->state == TABLE_SLOT_USED);ties the test toTableSlotinternals. Counting iterations is sufficient to validate the iterator contract and reduces coupling. If you want an extra check, assertit.current != NULLinstead.If
TableIterator.currentis intended to be public API, consider documenting that guarantee incommon/tableiterator.h.tests/test_syntax_definition.c (2)
40-66: Nice negative-path coverage; consider one more case.Current cases cover missing/invalid start and invalid end. Add a test for missing
color(if optional) or invalid type to lock behavior, and one for a non-empty but whitespace-onlystart.
111-175: Great table-driven error tests; add exact-root and missing-name scenarios.
- Add a case where
[block:rootX]exists but[block:root]doesn’t to ensure exact-match semantics for root detection.- Add a case where
[meta]exists butnamekey is missing to validate graceful handling (no crash).If exact match for “root” is required, update implementation accordingly (see comment in src/syntax/definition.c Lines 176-178).
src/syntax/definition.c (1)
250-291: API polish: set an error whentable == NULL.Currently returns NULL without touching
*error. Safer to set a code/message for callers.Example:
- if (!table) { - return NULL; - } + if (!table) { + if (error) { + *error = ERROR(SYNTAXDEFINITION_NO_META, "NULL table passed."); + } + return NULL; + }Adjust the error code if a better one exists in your enum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CMakeLists.txt(1 hunks)src/common/iniparser.c(1 hunks)src/common/iniparser.h(1 hunks)src/common/string.c(1 hunks)src/common/string.h(1 hunks)src/common/table.c(4 hunks)src/common/table.h(1 hunks)src/common/tableiterator.c(1 hunks)src/common/tableiterator.h(1 hunks)src/syntax/definition.c(1 hunks)src/syntax/definition.h(4 hunks)tests/CMakeLists.txt(1 hunks)tests/test_syntax_definition.c(1 hunks)tests/test_table.c(1 hunks)tests/test_tableiterator.c(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
src/common/tableiterator.h
[error] 4-4: 'sys/types.h' file not found
(clang-diagnostic-error)
🔇 Additional comments (5)
src/common/iniparser.h (1)
60-62: Colon in keys/sections: confirm behavioral intent and coverage.Allowing ':' in KEY_CHAR affects both keys and section names. Implementation aligns (see is_key_char). Please confirm tests include:
- section names with ':' (e.g., [lang:sh])
- assignments with ':' in keys
- no regression where ':' was previously treated specially.
src/common/iniparser.c (1)
144-146: is_key_char updated to include ':': add focused tests.Change is consistent with the grammar. Add tests for:
- Keys starting/ending with ':'
- Multiple ':' within a key
- ':' adjacent to '=' not misparsed.
src/syntax/definition.h (1)
73-75: All call sites verified and correctly updated.The comprehensive search confirms that all occurrences of
SyntaxDefinition_FromTablein the codebase (2 actual call sites in tests) have been updated to pass the newerrorparameter. No outdated 1-argument calls remain. The breaking signature change is complete.src/common/table.c (1)
223-226: Defensive empty-table guards are correct and properly exported.The function
Table_GetUsageis declared in src/common/table.h (line 103) and actively used elsewhere in the codebase (src/syntax/definition.c:126), confirming the API is properly exported and integrated. The early-return guards prevent accessing find_slot on empty tables—solid defensive pattern.Also applies to: 242-245, 264-267, 279-282, 290-295
tests/test_syntax_definition.c (1)
7-21: Block creation tests look solid.Good coverage for simple and start+end cases; error deinit is handled.
| if (!start_regex) { | ||
| set_error(error, | ||
| SYNTAXDEFINITION_BLOCK_NO_START_REGEX, | ||
| String_Format("Block \"%s\" has no start regex defined.", start_regex, name) | ||
| ); | ||
| free(block->name); | ||
| free(block); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Varargs misuse in error formatting (UB) when start regex is missing.
Format string expects 1 %s, but 2 arguments are passed and the first is start_regex (NULL). This is undefined behavior and can crash.
Apply this fix:
- set_error(error,
- SYNTAXDEFINITION_BLOCK_NO_START_REGEX,
- String_Format("Block \"%s\" has no start regex defined.", start_regex, name)
- );
+ set_error(error,
+ SYNTAXDEFINITION_BLOCK_NO_START_REGEX,
+ String_Format("Block \"%s\" has no start regex defined.", name)
+ );📝 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.
| if (!start_regex) { | |
| set_error(error, | |
| SYNTAXDEFINITION_BLOCK_NO_START_REGEX, | |
| String_Format("Block \"%s\" has no start regex defined.", start_regex, name) | |
| ); | |
| free(block->name); | |
| free(block); | |
| return NULL; | |
| } | |
| if (!start_regex) { | |
| set_error(error, | |
| SYNTAXDEFINITION_BLOCK_NO_START_REGEX, | |
| String_Format("Block \"%s\" has no start regex defined.", name) | |
| ); | |
| free(block->name); | |
| free(block); | |
| return NULL; | |
| } |
🤖 Prompt for AI Agents
In src/syntax/definition.c around lines 63 to 71, the String_Format call passes
two arguments while the format only expects one and the first argument is
start_regex (which is NULL) causing undefined behavior; change the String_Format
invocation to pass only the block name (e.g. String_Format("Block \"%s\" has no
start regex defined.", name)) so the format specifiers and arguments match and
no NULL is passed into the varargs, then keep the existing frees and return NULL
as before.
* 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 String_Split() and String_Trim() with tests * add example syntax definition * Feat/Implement Syntax Definitions (#84) * add SyntaxBlockDef and SyntaxDefinition declarations * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * Add table iteration API and improve parsing robustness (#83) * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * WIP: implement definition loader * add Table_GetUsage() * allow ':' in INI file key names * fix location of tests * fix typo in Doxygen comment * add NULL guard to String_Take() * Add early-exit guards to table lookup functions (#80) * Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete() * add functiond to create SyntaxDefinition_FromTable() with tests. More tests needed * additional tests (still not enough) * fix: free correct regex in regex_end branch in SyntaxBlockDef_FromTable() * fix: wrong numbe of arguments in String_Format() call in SymtaxBlockDef_FromTable() * add explanation * fix correct recognition of root block * fix memory lealk * add NULL guard for strdup in init_definition() * fix last fix * add additional test * add documentation * Feat/Syntax Highlighting: Add module to generate Highlighting information (#85) * add Stack_Copy() and Stack_IsEmpty() * fix: return copy in Stack_Copy() * implement syntax highlighting * add Table_CreateCustom() and Table_CreatePtr() * fix minor typing bugs * add Stack_Create(), Stack_Destroy() * fix but in find_first_child to actually return the child with the first occurence * automatically add patterns to end only_start blocks and to start/end root * fix memory leak produced in last commit * fix tests * fix bug in SyntaxHighllighting_HighlightString(): check if child or block end comes first * add basic tests * use root block it open_blocks parameter is NULL in SyntaxHighlighting_HighlightString() * add random tests * handle src->capacity == 0 in Stack_Copy() * add NULL pointer guard before using the key_free_func() function pointer in the Table module * fix error message for end_regex compilation error in SyntaxBlockDef_FromTable() * correct inlcudes * fix typo * add missing includes * change examples in documentation * change "key == NULL" to "!key" for non pointer keys * do not shadow parameter match in find_first_block() * change first parameter name in SyntaxHighlighting_HighlightString() for consitency * Cache regex matches and add ends_on option for Syntax definitions (#86) * add const to parameter if function does not modify it. add Buffer_Has_Space(), Buffer_Clear() * cache regex results for better performance * add "ends_on" (INI) property to SyntaxBlockDef * implement ends_on in the SyntaxHighlight modul * remove the check if the ends_on block is allowed inside the surrounding block. this leads to more flexibility and better performance * add additional (but just a single) test for the new feature * add Stack_CopyTo() * save open_blocks_at_begin and open_blocks_at_end to SyntaxHighlightingString instances ins SyntaxHighlighting_HighlightString() * Add Syntax Highlighting binding for TextBuffer (#87) * add TableIterator (#79) * add TableIterator * change include that provides ssize_t * add additional tests * make current slot pointer const * include stdbool.h for bool * Add early-exit guards to table lookup functions (#80) * Fix bug in reading operations on an empty table * add check for empty table also to Table_Delete() * Add CodeRabbit configuration template Added a comprehensive configuration template for CodeRabbit, including global settings, review settings, chat configurations, knowledge base settings, and code generation options. * add SyntaxHighlightingBinding to connect SyntaxHighlighting with the TextBuffer/TextLayout * fix some obvious bugs in SyntaxHighlighingBinding * fix memory leak * add basic test * add Stack_Size() * rename test function * add Stack_CopyTo() * save open_blocks_at_begin and open_blocks_at_end to SyntaxHighlightingString instances ins SyntaxHighlighting_HighlightString() * adept code to the changed SyntaxHighlighingString scheme * improve tests * Compare the open_blocks_at_begin with the open_blocks parameter for an early exit int SyntaxHighlighting_HighlightString() * add testcases and improve test framework's flexibility * fix coderabbit config * fix resizing in Stack_CopyTo() * add NULL guard to SyntaxHighlighting_HighlightString() * change multiline string to valid C string * change multiline strings to valid C strings * set shs->tags_capacity correctly in SyntaxHighlioghtingString_Create() * make argv0 available through Config * Add File_Exits(), File_OpenConfig() and File_OpenProjectFile() * change defautl width * fix wrong function calls * use new File_LoadConfig() function * remove unused variable * check if snprintf() truncated the path * fix function declaration of File_OpenProjectFile() * Implement a loader function to load a complete SyntaxHighlighting engine from an INI file Enable compile_commands.json and improve syntax handling (#88) * propagate PATH_MAX by file.h * create compile_commands.json (for correct using include paths in vscode) * add create and destroy functions to SyntaxHighlighting * SyntaxHighlighting now takes the ownership of the used SyntaxDefinition * must not destroy SyntaxDefinition anymore since this is handled by SyntaxHighlighting * fix bug in find_project_file() * implement the SyntaxHighlighting_LoadFromFile() function and tests * add NULL guard * Initialize error properly in SyntaxHighlighting_LoadFromFile() * guard againsst line == NULL and handle last_line == NULL in SyntaxHighlightingBinding_Update() * fix bug in concat_paths() * add test_definition_error to TEST_LIST * Integrate SyntaxHighlighting to the editor (#89) * wip * link to the data directory in the build directory * example syntax definitions * massive changes to get syntax highlighting working... need to optimization and better organisation, lot of work arounds * comment out TESTFILE definition * destroy SyntaxHighlighting instance properly * fix Table_Get() call in draw_visual_line() to use the correct key * improve highlighting * add Config_SetSyntax(), Config_GetSyntax() * use getopt() to parse cmd arguments, take syntax type from cmd arguments * Trigger a full SyntaxHighlighting update once in the beginning * add hint that byte offset is used * update screenshot * improve markdown highlighting * remove TESTFILE debugging stuff * handle the case that no highlighting is selected * deinitialize IniParser at ealry exit if c == NULL in Config_LoadIni() * remove double deinitialization of TextSelection in editor_destroy() * fix parse_arguments() and update print_help() * fix critical bug in Table_Set/Get(): change state of tombstones when reusing them * anchor section block start to newline * fix comment * fix off-by-one bug in Buffer_HasSpace() * guard against NULL in SyntaxHighlightingBinding_UpdateAll() * update syntax definitions * fix off-by-one bug in SyntaxHighlightingString_AddTag() * fix potential int overflow * add comment * minor change to const correctness in cpy_ptr * update readme * fix intendation
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores