Implement a loader function to load a complete SyntaxHighlighting engine from an INI file Enable compile_commands.json and improve syntax handling

[defname]

Summary by CodeRabbit

• New Features

  • Load syntax highlighting definitions from external files with detailed error reporting and a loader API.

• Bug Fixes

  • Added null-pointer guards in layout/update code to prevent potential crashes.

• Refactor

  • Improved lifecycle management for syntax highlighting and more robust executable-path resolution.

• Tests

  • Added tests covering syntax loader success and failure cases.

• Chores

  • Enabled generation of compile_commands.json for improved build diagnostics.

[coderabbitai]

Walkthrough

This PR enables compile_commands.json in CMake, adjusts executable path resolution, converts SyntaxHighlighting to take ownership of SyntaxDefinition with Create/Destroy APIs, adds a loader to read syntax .ini files with detailed errors, adds null guards in bindings, and updates tests accordingly.

Changes

Cohort / File(s)	Summary
Build Configuration CMakeLists.txt	Enables compile_commands.json generation via set(CMAKE_EXPORT_COMPILE_COMMANDS on).
File I/O Path Resolution src/io/file.c, src/io/file.h	Refactors exe path resolution: uses a separate exe_abs_path buffer for realpath() then dirname(); adds limits.h inclusion to expose PATH_MAX.
Syntax Highlighting Core API src/syntax/highlighting.c, src/syntax/highlighting.h	Changes def from const to owned SyntaxDefinition *; SyntaxHighlighting_Init now accepts non-const SyntaxDefinition *; adds SyntaxHighlighting_Create and SyntaxHighlighting_Destroy; Deinit now destroys owned def.
Syntax Highlighting Loader (New) src/syntax/loader.c, src/syntax/loader.h	Adds SyntaxHighlighting_LoadFromFile(format, error) and SyntaxHighlightingLoaderError variants; implements staged file open → read → parse → SyntaxDefinition_FromTable → SyntaxHighlighting_Create with granular error reporting and cleanup.
Syntax Bindings Guard src/syntax/textlayoutbindings.c	Adds early-return null-checks in SyntaxHighlightingBinding_Update and normalizes last_line if NULL.
Test Suite Updates tests/test_syntax_highlighting.c, tests/test_syntax_textlayoutbindings.c	Removed explicit SyntaxDefinition_Destroy(...) calls from tests/fixtures (teardown no longer destroys definitions there).
Syntax Loader Tests (New) tests/test_syntax_loader.c	New unit tests for loader: setup/teardown helpers for data/syntax, tests for success, NULL input, file-not-found, parse errors, and definition errors.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant Loader as SyntaxHighlighting_LoadFromFile
    participant File as File I/O
    participant Parser as IniParser
    participant DefBuilder as SyntaxDefinition_FromTable
    participant HL as SyntaxHighlighting_Create

    Caller->>Loader: LoadFromFile(format, error)
    
    rect rgb(200, 220, 255)
    Note over Loader: Validation & Initialization
    Loader->>Loader: Initialize error state<br/>Check format != NULL
    end
    
    rect rgb(220, 255, 220)
    Note over Loader: File Read Phase
    Loader->>File: Open data/syntax/<format>.ini
    alt File Open Success
        Loader->>File: Read file content
        alt Read Success
            File-->>Loader: Content buffer
        else Read/IO Error
            Loader->>Loader: Set error (FILE_READ_ERROR)
            Loader-->>Caller: NULL
            rect rgb(255, 200, 200)
            Note over Loader: Error path
            end
        end
    else File Not Found
        Loader->>Loader: Set error (FILE_NOT_FOUND)
        Loader-->>Caller: NULL
        rect rgb(255, 200, 200)
        Note over Loader: Error path
        end
    end
    end
    
    rect rgb(255, 240, 200)
    Note over Loader: Parse Phase
    Loader->>Parser: Parse INI content to Table
    alt Parse Success
        Parser-->>Loader: Table
    else Parse Error
        Loader->>Loader: Copy parse error to error.parsing_error
        Loader->>Loader: Set error (PARSE_ERROR)
        Loader-->>Caller: NULL
        rect rgb(255, 200, 200)
        Note over Loader: Error path
        end
    end
    end
    
    rect rgb(240, 255, 240)
    Note over Loader: Definition Build Phase
    Loader->>DefBuilder: SyntaxDefinition_FromTable(Table)
    alt Definition Success
        DefBuilder-->>Loader: SyntaxDefinition
    else Definition Error
        Loader->>Loader: Copy def error to error.def_error
        Loader->>Loader: Set error (DEFINITION_ERROR)
        Loader-->>Caller: NULL
        rect rgb(255, 200, 200)
        Note over Loader: Error path
        end
    end
    end
    
    rect rgb(220, 240, 255)
    Note over Loader: Create Phase
    Loader->>HL: SyntaxHighlighting_Create(SyntaxDefinition)
    HL-->>Loader: SyntaxHighlighting*
    end
    
    Loader-->>Caller: SyntaxHighlighting* (ownership transferred)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Review ownership/API change in src/syntax/highlighting.h and ensure all callers handle non-const ownership correctly.
• Inspect src/syntax/loader.c error paths for resource leaks and correctness of error propagation.
• Verify src/io/file.c realpath/dirname buffering and bounds handling around PATH_MAX.
• Confirm test changes (removed SyntaxDefinition_Destroy) align with new ownership semantics and do not introduce leaks.

Possibly related PRs

• Feat/Syntax Highlighting: Add module to generate Highlighting information #85 — Direct changes to the same SyntaxHighlighting API surface (Init signature, Create/Destroy).
• Add Syntax Highlighting binding for TextBuffer #87 — Related edits touching syntax highlighting bindings and tests.
• Add table iteration API and improve parsing robustness #83 — Changes involving SyntaxDefinition_FromTable / destroy semantics used by the loader.

Poem

🐰 I hopped through paths both real and neat,
Found INI secrets tucked in a sheet,
I took a def with careful paw,
Built highlighting, checked each flaw,
Now tests and compiles hum — what a treat! 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 17.39% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "Implement a loader function to load a complete SyntaxHighlighting engine from an INI file" directly corresponds to the primary changes in the changeset. The new src/syntax/loader.h and src/syntax/loader.c files introduce the SyntaxHighlighting_LoadFromFile function along with comprehensive error handling, and tests/test_syntax_loader.c validates this functionality. Supporting changes to src/syntax/highlighting.c and highlighting.h (implementing Create/Destroy functions and ownership semantics) enable the loader to function properly, while infrastructure changes to CMakeLists.txt and src/io/file.* provide necessary compilation and path-resolution support. The title is concise, specific, and accurately conveys the main objective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/sh-loader

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

🧹 Nitpick comments (9)

CMakeLists.txt (1)

8-9: Enabling compile_commands.json looks good

Consider using uppercase ON for consistency and, if your build dir ≠ source dir, add a post-config step to copy/symlink compile_commands.json to the source root so editors find it.

src/syntax/highlighting.c (1)

118-121: Add null‑guard to Destroy (optional)

Defensive free if API might receive NULL.

Apply:

-void SyntaxHighlighting_Destroy(SyntaxHighlighting *hl) {
-    SyntaxHighlighting_Deinit(hl);
-    free(hl);
-}
+void SyntaxHighlighting_Destroy(SyntaxHighlighting *hl) {
+    if (!hl) return;
+    SyntaxHighlighting_Deinit(hl);
+    free(hl);
+}

src/syntax/highlighting.h (2)

85-91: Document ownership prominently in Init

Consider adding “def must be heap‑allocated; sh takes ownership and will destroy it.”

---

95-105: Create/Destroy API addition looks good

Suggest documenting that Destroy also frees the owned def.

src/syntax/loader.h (3)

4-6: Include what you use: add the header that declares SyntaxDefinitionError

This header uses SyntaxDefinitionError but doesn’t include its declaration explicitly. Relying on transitive includes is brittle.

Apply:

 #include "highlighting.h"
 #include "common/iniparser.h"
+#include "definition.h"

---

25-40: Fix docs: typos, type name, path, and clarify error contract

Several typos and one wrong type name; also clarify that error must be non-NULL.

 /**
  * @brief Load a syntax definition and create the highlight engine.
  * 
- * Load the syntax definition for `format` from data/sytnax/<format>.ini
- * (from inside a project's folder) and create the `SyntaxHighlight` instance.
+ * Load the syntax definition for `format` from data/syntax/<format>.ini
+ * (from inside the project's folder) and create the `SyntaxHighlighting` instance.
  * 
  * @param format Format to load (something like "ini", "md").
- * @param error Will be set in the case of an error and need to be deinitialized by the caller.
+ * @param error Out-parameter for error details. Must not be NULL. When an error occurs,
+ *              it will be populated and must be deinitialized by the caller.
  * @returns
- * The newly created SyntaxHighlighting Instance (ownership transfers to the caller).
- * NULL there is definition file or if an error occured.
- * In the case of an error `error` will be set and need to be deinitialized by the caller. 
+ * The newly created SyntaxHighlighting instance (ownership transfers to the caller),
+ * or NULL if no definition file exists or if an error occurred.
+ * On error, `error` will be populated and must be deinitialized by the caller.
  * 
- * Note theat `format` needs to be exactly the same as the filename.
+ * Note that `format` needs to be exactly the same as the filename.
  * The convention is to use lower case.
  */

---

16-20: Public API coupling to ParsingError

Exposing ParsingError in a public error struct ties this API to iniparser’s internal layout. Consider a stable copy (your own error struct) and translate fields to it.

src/syntax/loader.c (2)

6-11: Make deinit robust to NULL and double-free

Guard NULL, and null out the message after free to avoid accidental double-free on reuse.

 void SyntaxHighlightingLoaderError_Deinit(SyntaxHighlightingLoaderError *error) {
-    SyntaxDefinitionError_Deinit(&error->def_error);
-    if (error->parsing_error.message) {
-        free(error->parsing_error.message);
-    }
+    if (!error) return;
+    SyntaxDefinitionError_Deinit(&error->def_error);
+    if (error->parsing_error.message) {
+        free(error->parsing_error.message);
+        error->parsing_error.message = NULL;
+    }
+    error->code = SYNTAX_LOADER_NO_ERROR;
 }

---

47-51: Review comment is valid but describes preventive refactoring, not a current bug

The ParsingError struct contains only three fields: int line, int column, and char *message (one pointer). The current code correctly works because:

1. Line/column are POD fields—safe to shallow copy
2. Message is explicitly deep-copied via strdup() before IniParser_Deinit() is called

The review's concern is valid from a maintenance perspective: if a future pointer field is added to ParsingError without updating this code, it would dangle. The suggested refactor improves defensiveness and clarity.

Additionally, there's a minor inefficiency: IniParser_GetError() is called twice on consecutive lines when once would suffice.

The suggested approach of copying only POD fields and explicitly handling pointer fields is sound defensive programming.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b17766b and d959c68.

📒 Files selected for processing (11)

• CMakeLists.txt (1 hunks)
• src/io/file.c (1 hunks)
• src/io/file.h (1 hunks)
• src/syntax/highlighting.c (2 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)
• tests/test_syntax_highlighting.c (0 hunks)
• tests/test_syntax_loader.c (1 hunks)
• tests/test_syntax_textlayoutbindings.c (0 hunks)

💤 Files with no reviewable changes (2)

• tests/test_syntax_highlighting.c
• tests/test_syntax_textlayoutbindings.c

🧰 Additional context used

🧬 Code graph analysis (6)

src/syntax/highlighting.c (1)

src/syntax/definition.c (1)

• SyntaxDefinition_Destroy (372-386)

src/io/file.c (1)

src/common/config.c (1)

• Config_GetExePath (89-91)

src/syntax/highlighting.h (1)

src/syntax/highlighting.c (4)

• SyntaxHighlighting_Init (93-96)
• SyntaxHighlighting_Deinit (98-107)
• SyntaxHighlighting_Create (109-116)
• SyntaxHighlighting_Destroy (118-121)

src/syntax/loader.c (5)

src/syntax/definition.c (2)

• SyntaxDefinitionError_Deinit (36-38)
• SyntaxDefinition_FromTable (324-370)

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 (223-229)

src/syntax/highlighting.c (1)

• SyntaxHighlighting_Create (109-116)

src/syntax/loader.h (1)

src/syntax/loader.c (2)

• SyntaxHighlightingLoaderError_Deinit (6-11)
• SyntaxHighlighting_LoadFromFile (14-68)

tests/test_syntax_loader.c (3)

src/common/config.c (3)

• Config_GetExePath (89-91)
• Config_Init (40-49)
• Config_Deinit (51-58)

src/syntax/loader.c (2)

• SyntaxHighlighting_LoadFromFile (14-68)
• SyntaxHighlightingLoaderError_Deinit (6-11)

src/syntax/highlighting.c (1)

• SyntaxHighlighting_Destroy (118-121)

🪛 Clang (14.0.6)

src/io/file.h

[error] 19-19: 'limits.h' file not found

(clang-diagnostic-error)

🔇 Additional comments (9)

src/io/file.c (1)

70-76: Excellent refactoring for safer path resolution.

The change from in-place buffer usage to separate buffers for realpath and dirname improves both clarity and safety. The new approach correctly handles dirname()'s POSIX behavior (may modify input buffer and return pointer to internal storage or modified buffer). The returned pointer is used immediately within its valid scope, making this implementation safe.

src/syntax/highlighting.c (2)

102-105: Deinit now frees hl->def (breaking change)

Confirm no external owner frees def separately.

If any legacy code used borrowed defs, update it to not destroy def.

---

93-97: Ownership transfer verified—no double-free patterns detected

All call sites properly call SyntaxHighlighting_Deinit after passing def to SyntaxHighlighting_Init. The ownership transfer is correctly managed: Deinit takes ownership and destroys def via SyntaxDefinition_Destroy. No cases found where def is destroyed twice or accessed after transfer.

src/syntax/highlighting.h (1)

76-83: Ownership change is clear

Field switched to owned SyntaxDefinition*. Matches impl.

src/syntax/loader.h (1)

41-41: Document and enforce non-NULL error

The implementation dereferences error. Make non-NULL a documented precondition (see doc fix) and also enforce it in code.

Would you like me to add an assertion/guard in the implementation as shown in my loader.c diff below?

src/syntax/loader.c (4)

33-39: Good cleanup on read failure

File_Close is called on read error before returning. Nice.

---

58-64: OK: Definition error path cleans up Table and sets code

This branch looks correct and leak-free.

---

66-68: Return ownership clarity

Returning SyntaxHighlighting_Create(def) correctly transfers ownership; ensure callers destroy via SyntaxHighlighting_Destroy.

Do tests cover destroy on both success and failure paths?

---

23-31: Function appears to be dead code; format cannot receive user input in current application

After comprehensive codebase analysis, SyntaxHighlighting_LoadFromFile is only called from test files (5 calls in tests/test_syntax_loader.c with hardcoded strings). No calls exist in production code—including src/main.c, app initialization, or editor modules. The main application flow parses a filename argument but never attempts to load syntax highlighting, making the function unreachable with user-controlled input.

The snprintf truncation check remains good defensive programming practice. However, the path traversal concern is currently theoretical since format cannot originate from user input.

Verify whether this function is intended for future use and confirm the planned source of the format parameter.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

src/syntax/loader.c (1)

1-4: Still missing <limits.h> for PATH_MAX.

The prior review flagged that PATH_MAX (used at line 32) requires <limits.h>, but this header was not added. Without it, builds may fail on stricter toolchains.

Apply this diff:

 #include "loader.h"
 #include <string.h>
 #include <stdio.h>  // snprintf
+#include <limits.h>  // PATH_MAX
 #include "io/file.h"

🧹 Nitpick comments (2)

src/syntax/loader.c (2)

12-17: Add NULL guard for defensive programming.

While the current usage always calls Init before Deinit, this is a public API and should handle NULL gracefully.

Apply this diff:

 void SyntaxHighlightingLoaderError_Deinit(SyntaxHighlightingLoaderError *error) {
+    if (!error) {
+        return;
+    }
     SyntaxDefinitionError_Deinit(&error->def_error);
     if (error->parsing_error.message) {
         free(error->parsing_error.message);
     }
 }

---

65-76: Consider documenting ownership transfer.

The code correctly creates the engine from def (line 75), and based on the API contract SyntaxHighlighting_Create takes ownership. However, the ownership transfer is implicit, and there's no NULL check after Create.

While the current implementation is correct (since SyntaxHighlighting_Create calls logFatal on allocation failure), consider adding a comment to clarify ownership:

     Table_Destroy(table);
 
-    // 5. create the highlighting engine
+    // 5. create the highlighting engine (transfers ownership of def)
     return SyntaxHighlighting_Create(def);
 }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d959c68 and 4f6f7ec.

📒 Files selected for processing (3)

• src/syntax/loader.c (1 hunks)
• src/syntax/textlayoutbindings.c (1 hunks)
• tests/test_syntax_loader.c (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• src/syntax/textlayoutbindings.c
• tests/test_syntax_loader.c

🧰 Additional context used

🧬 Code graph analysis (1)

src/syntax/loader.c (5)

src/syntax/definition.c (2)

• SyntaxDefinitionError_Deinit (36-38)
• SyntaxDefinition_FromTable (324-370)

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 (223-229)

src/syntax/highlighting.c (1)

• SyntaxHighlighting_Create (109-116)

🔇 Additional comments (3)

src/syntax/loader.c (3)

6-9: LGTM—clean initialization.

The memset zero-initializes all fields (including def_error), addressing the prior critical concern about uninitialized state.

---

20-29: LGTM—proper validation and initialization.

The NULL guard for error (lines 21-23) and zero-initialization via Init (line 24) correctly address the prior critical issue.

---

31-47: LGTM—clean file handling.

File open/read/close operations are properly sequenced, with File_Close correctly called in both success and error paths.