-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix warning for Elixir 1.18 #36
Conversation
WalkthroughThe pull request introduces updates to the Elixir CI workflow and the Kinda library's code generation mechanism. The changes focus on expanding the CI matrix to include a new Elixir version, adding code formatting checks, and refining the NIF (Native Implemented Function) code generation process. The modifications enhance the project's testing coverage and improve the macro-based code generation approach for native interfaces. Changes
Sequence DiagramsequenceDiagram
participant Workflow as CI Workflow
participant CodeGen as Kinda.CodeGen
participant NIF as NIF Module
Workflow->>CodeGen: Generate NIF AST
CodeGen-->>NIF: Evaluate AST
NIF-->>CodeGen: Return Module and Function
CodeGen-->>Workflow: Complete NIF Generation
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 (4)
lib/kinda_codegen.ex (2)
Line range hint
32-47
: Ensure correct struct-based pattern matching in NIFDecl.
It’s good practice to verify that the%NIFDecl{}
struct includes all required fields for correct pattern matching. If future fields are introduced to the struct, their default handling or matching must be updated in this case block.
71-77
: NIF error handling improvements.
Returning :erlang.nif_error(:not_loaded) ensures a graceful failure in production. However, it might be beneficial to log or track these errors in a more robust manner.kinda_example/lib/kinda_exmaple_nif.ex (1)
2-5
: Module attribute used for macro invocation.
Declaring “@nifs use Kinda.CodeGen…” is a stylistic choice that can improve readability by centralizing the arguments, but be mindful that it might confuse new contributors expecting a standard “use” at the top level. Consider adding a brief comment explaining the reason for using a module attribute if more contributors are expected to work on this code..github/workflows/elixir.yml (1)
26-29
: Add matrix exclusion rationale.
Excluding the “otp: 24.2” and “elixir: 1.18.0” combination is valid if known incompatibilities exist. Documenting the reason for this exclusion can help future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/elixir.yml
(1 hunks)kinda_example/lib/kinda_exmaple_nif.ex
(1 hunks)lib/kinda_codegen.ex
(4 hunks)
🔇 Additional comments (3)
lib/kinda_codegen.ex (3)
13-14
: Confirm consistent usage of return tuple.
These two lines rely on tuple-destructuring the result of nif_ast (i.e., {ast, mf}). Ensure all upstream references to the AST and the mf align with this new approach so calling code doesn't rely on the previously piped approach.
30-31
: Double-check potential duplication of NIF declarations.
Here, nifs are concatenated with extra_kind_nifs. If both contain overlapping or identical declarations, it may lead to conflicts or unexpected behavior. Consider deduplicating or verifying uniqueness before concatenation.
57-67
: Conditionally define a wrapper function.
By checking if nif_name != wrapper_name, a second function definition is introduced only when needed. This is good, but watch out for edge cases involving name collisions (e.g., if wrapper_name is changed dynamically). A test covering that scenario would be beneficial.
Summary by CodeRabbit
New Features
1.18.0
.Bug Fixes
Refactor
use Kinda.CodeGen
directive in theKindaExample.NIF
module for clarity.Kinda.CodeGen
module.Chores
Kinda
application from0.9.4-dev
to0.9.5-dev
.