Skip to content

Remove redundant module exports#50

Merged
jackalcooper merged 5 commits into
mainfrom
remove-module-exports
Oct 13, 2025
Merged

Remove redundant module exports#50
jackalcooper merged 5 commits into
mainfrom
remove-module-exports

Conversation

@jackalcooper
Copy link
Copy Markdown
Contributor

@jackalcooper jackalcooper commented Oct 13, 2025

Summary by CodeRabbit

  • New Features

    • Automatic detection of the Erlang/ERTS include path during build when the environment variable is not set, improving out-of-the-box setup and editor integration.
  • Refactor

    • Simplified NIF integration and internal wiring to reduce configuration overhead. No changes to public APIs or behavior.
  • Chores

    • Version bumped to 0.10.7-dev.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 13, 2025

Walkthrough

Removes the standalone erl_nif Zig module and rewires NIF usage through src/kinda.zig’s direct @cImport of erl_nif.h. Updates imports in dependent files. Adds build-time detection of ERTS include path via calling erl when ERTS_INCLUDE_DIR is unset. Bumps version in mix.exs.

Changes

Cohort / File(s) Summary
Build and versioning
build.zig, mix.exs
build.zig: drops explicit erl_nif/beam module registration; adds conditional ERTS include path discovery by invoking erl when ERTS_INCLUDE_DIR is unset, wiring include path to kinda. mix.exs: version bumped 0.10.6-dev → 0.10.7-dev.
NIF import rewiring
src/kinda.zig, src/beam.zig, src/result.zig
kinda.zig: replaces Zig module import of erl_nif with direct @cImport of "erl_nif.h"; adjusts beam import to "beam.zig". beam.zig: switches NIF import to kinda.erl_nif. result.zig: updates imports to use beam.zig and kinda.erl_nif.
Removal of erl_nif module
src/erl_nif.zig
Deletes the erl_nif helper module; removes pub const c @cImport wrapper and docs, eliminating the prior Zig-level NIF alias.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant B as build.zig
  participant Env as Environment
  participant ERL as erl (ERTS)
  participant K as kinda module

  Dev->>B: zig build
  B->>Env: read ERTS_INCLUDE_DIR
  alt ERTS_INCLUDE_DIR is set
    B->>K: add include path from env
  else ERTS_INCLUDE_DIR is not set
    B->>ERL: run erl to query ERTS include path
    ERL-->>B: returns include path
    B->>K: add discovered include path
  end
  note over K: @cImport("erl_nif.h") used by kinda and dependents
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • Migrate to Zig 0.15.1 #46 — Also refactors erl_nif/beam wiring across src/kinda.zig, src/beam.zig, src/erl_nif.zig, and result.zig.
  • 0.10.3-dev #44 — Adjusts mix.exs version string, overlapping this PR’s version bump.

Poem

I nibbled wires, tidy and neat,
Swapped my imports—what a treat! 🐇
Called on erl to find the way,
Include paths set for build-time play.
The old NIF burrow’s sealed and snug—
New tunnels through kinda, warm and snug. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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 title “Remove redundant module exports” correctly highlights the removal of erl_nif and beam export wiring, which is a genuine part of the changeset, but it omits the added dynamic Erlang include path logic that is also central to this update. Because it refers to a real aspect of the changeset without capturing the main combined intent, it is partially related.
✨ 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 remove-module-exports

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

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

@jackalcooper jackalcooper merged commit 3227cf3 into main Oct 13, 2025
5 checks passed
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/beam.zig (2)

759-767: Off-by-one in get_tuple slice

You drop the last element by slicing to length-1.

-        return term_list[0..(length - 1)];
+        return term_list[0..@intCast(length)];

1059-1073: make_c_string: wrong iterator target and includes NUL byte

Loop iterates over the wrong symbol and returns a binary including the trailing NUL.

-pub fn make_c_string(environment: env, val: [*c]const u8) term {
-    const result: e.ERL_NIF_TERM = undefined;
-    var len: usize = 0;
-
-    // first get the length of the c string.
-    for (result, 0..) |chr, i| {
-        if (chr == 0) {
-            break;
-        }
-        len = i;
-    }
-
-    // punt to the slicing function.
-    return make_slice(environment, val[0 .. len + 1]);
-}
+pub fn make_c_string(environment: env, val: [*c]const u8) term {
+    var len: usize = 0;
+    // compute strlen(val) without including the trailing NUL
+    while (val[len] != 0) : (len += 1) {}
+    return make_slice(environment, val[0..len]);
+}
src/result.zig (1)

14-14: Wrong calling convention enum

Use .C (uppercase), not .c.

-        fn exported(env: beam.env, n: c_int, args: [*c]const beam.term) callconv(.c) beam.term {
+        fn exported(env: beam.env, n: c_int, args: [*c]const beam.term) callconv(.C) beam.term {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09d671c and f485244.

📒 Files selected for processing (6)
  • build.zig (1 hunks)
  • mix.exs (1 hunks)
  • src/beam.zig (1 hunks)
  • src/erl_nif.zig (0 hunks)
  • src/kinda.zig (1 hunks)
  • src/result.zig (1 hunks)
💤 Files with no reviewable changes (1)
  • src/erl_nif.zig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: otp25.0-ex1.18.0 / ubuntu-22.04
  • GitHub Check: otp25.0-ex1.16.2 / ubuntu-22.04
  • GitHub Check: otp24.2-ex1.16.2 / ubuntu-22.04
🔇 Additional comments (3)
mix.exs (1)

7-7: Version bump looks good

No issues found.

src/kinda.zig (1)

1-4: Re-export approach is fine, but avoid import cycles

Keeping erl_nif here is OK. Ensure beam.zig and result.zig import erl_nif via @cImport themselves (not via kinda.zig) to prevent cycles described in their files.

build.zig (1)

8-21: Make ERTS include discovery more robust (trim, absolute path, skip missing erl)

  • Trim trailing newlines from b.run output with std.mem.trimRight.
  • Use .absolute for addIncludePath instead of .cwd_relative.
  • Wrap b.run in catch to skip when erl isn’t available.
-        const erlang_include_path = b.run(&argv);
-        kinda.addIncludePath(.{ .cwd_relative = erlang_include_path });
+        const raw = b.run(&argv) catch { return; };
+        const erlang_include_path = std.mem.trimRight(u8, raw, "\r\n");
+        if (erlang_include_path.len != 0) {
+            kinda.addIncludePath(.{ .absolute = erlang_include_path });
+        }

Comment thread src/beam.zig
Comment thread src/result.zig
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant