Skip to content

Remove yield APIs#52

Merged
jackalcooper merged 3 commits into
mainfrom
rm-yield-apis
Oct 26, 2025
Merged

Remove yield APIs#52
jackalcooper merged 3 commits into
mainfrom
rm-yield-apis

Conversation

@jackalcooper
Copy link
Copy Markdown
Contributor

@jackalcooper jackalcooper commented Oct 26, 2025

Summary by CodeRabbit

  • Chores

    • Version bumped to 0.10.9-dev
  • Refactor

    • Simplified process identity handling
    • Removed yielding/native scheduling infrastructure
  • Bug Fixes / Reliability

    • Caller-acquisition now returns a clearer error when the calling process cannot be determined (replacing an ambiguous failure)

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 26, 2025

Walkthrough

Replaces thread-local process identity handling with a single public self(environment) !pid in src/beam.zig, adds an error variant "Fail to get calling process", and removes the yielding NIF scaffolding; also bumps project version in mix.exs to "0.10.9-dev".

Changes

Cohort / File(s) Summary
Version Update
mix.exs
Bumps project version from "0.10.8-dev" to "0.10.9-dev"
Process Identity & Yielding Refactor
src/beam.zig
Adds error "Fail to get calling process"; introduces pub fn self(environment: env) !pid; removes thread-local self holder and its setters (set_generic_self, set_threaded_self), removes generic_self and threaded_self; deletes yielding/NIF scaffolding (yield_info, Frame, YieldError, YieldState, YieldInfo, set_yield_response, yield()), and simplifies caller-PID acquisition control flow

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Erlang caller
    participant Beam as beam.zig

    Note over Caller,Beam #D6EAF8: New flow
    Caller->>Beam: call self(environment)
    Beam-->>Caller: pid | error("Fail to get calling process")
Loading
sequenceDiagram
    participant Caller as Erlang caller
    participant Beam as beam.zig (old)

    Note over Caller,Beam #FDEBD0: Previous (removed) flow
    Caller->>Beam: thread-local self lookup
    alt thread-local set
        Beam-->>Caller: pid
    else not set
        Beam-->>Caller: Function clause error (was)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20-25 minutes

  • Pay attention to all call sites of self/thread-local access throughout the codebase and tests.
  • Verify no remaining references to the removed yield/NIF types and functions.
  • Confirm error variant propagation and any pattern-matching of prior Function-clause errors are updated.

Possibly related PRs

Poem

🐰 I hopped through code at break of day,
Threadlocals tossed, a single way.
No yields to tumble, simpler call,
A tiny error, catching all.
Version tucked—onward we play. ✨

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 pull request title "Remove yield APIs" is directly and accurately related to the primary functional change in the changeset. The raw summary shows that a substantial portion of the changes involves removing yield-related public declarations and implementations from src/beam.zig, including yield_info, Frame, YieldError, YieldState, YieldInfo, set_yield_response, and the yield() function itself. The title is concise, specific, and clearly communicates the main objective of the PR to someone reviewing the commit history. While the PR also includes other changes such as a version bump and refactoring of the self function, the removal of yield APIs represents the core functional change being made.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch rm-yield-apis

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.

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: 0

🧹 Nitpick comments (1)
src/beam.zig (1)

706-713: Consider simplifying the return value.

The function correctly handles the case where the environment is not process-bound. However, since enif_self fills in the p variable that was passed as the second argument, and self_val is just a pointer to that same structure, you could simplify by returning p directly:

Apply this diff to simplify the implementation:

 pub fn self(environment: env) !pid {
     var p: pid = undefined;
-    if (e.enif_self(environment, @ptrCast(&p))) |self_val| {
-        return self_val.*;
+    if (e.enif_self(environment, @ptrCast(&p))) |_| {
+        return p;
     } else {
         return Error.@"Fail to get calling process";
     }
 }

This matches the pattern used in get_pid at line 690-697.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 52f27e9 and 156a6e2.

📒 Files selected for processing (1)
  • src/beam.zig (2 hunks)
⏰ 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). (1)
  • GitHub Check: otp25.0-ex1.16.2 / ubuntu-22.04
🔇 Additional comments (2)
src/beam.zig (2)

270-270: LGTM! Clear error variant added.

The new error variant "Fail to get calling process" is appropriately named and follows the existing error naming conventions in the Error enum.


706-713: The review comment is based on incorrect information about the changes.

The comprehensive search across the entire codebase found zero references to any of the supposedly removed APIs (generic_self, threaded_self, set_generic_self, set_threaded_self, yield_info, YieldError, YieldState, YieldInfo, set_yield_response, yield()). These functions do not exist in the repository. The code change at lines 706-713 introduces a new self() wrapper function around e.enif_self() for convenience—it is not a replacement for removed APIs.

Likely an incorrect or invalid review comment.

@jackalcooper jackalcooper merged commit 8121571 into main Oct 26, 2025
5 checks passed
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