refactor: extract napi_property_descriptro helpers#201
Conversation
Summary of ChangesHello @spiral-ladder, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on improving the modularity and reusability of N-API binding code by extracting common helper functions into a new, dedicated file. This refactoring centralizes the logic for generating N-API property descriptors, making the codebase cleaner and more maintainable by promoting a single source of truth for these utilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors helper functions for creating napi_property_descriptor instances into a new, dedicated file. This is a good change that improves code organization and reusability. I've identified a critical compilation error in the new file and suggested a fix. I've also recommended an improvement to make the function name extraction more robust by using @fnName instead of parsing compiler-internal type information, which also addresses line length style violations.
| comptime func: anytype, | ||
| ) napi.c.napi_property_descriptor { | ||
| const name = comptime fnName(func); | ||
| return .{ .utf8name = name.ptr, .getter = napi.wrapCallback(0, func) }; |
There was a problem hiding this comment.
The fnName function returns a [:0]const u8, which is a C-style string pointer. This pointer type does not have a .ptr field. Accessing name.ptr will result in a compilation error. You should use name directly, as it is already the pointer required for the utf8name field.
return .{ .utf8name = name, .getter = napi.wrapCallback(0, func) };
| comptime func: anytype, | ||
| ) napi.c.napi_property_descriptor { | ||
| const name = comptime fnName(func); | ||
| return .{ .utf8name = name.ptr, .method = napi.wrapCallback(argc_cap, func) }; |
There was a problem hiding this comment.
The fnName function returns a [:0]const u8, which is a C-style string pointer. This pointer type does not have a .ptr field. Accessing name.ptr will result in a compilation error. You should use name directly, as it is already the pointer required for the utf8name field.
return .{ .utf8name = name, .method = napi.wrapCallback(argc_cap, func) };
| const fq_name = @typeName(@TypeOf(func)); | ||
| const start_index = comptime std.mem.indexOf(u8, fq_name, "@typeInfo") orelse @compileError("Expected a @typeInfo"); | ||
| const underscore_index = comptime std.mem.indexOfScalar(u8, fq_name[start_index..], '_') orelse @compileError("Expected an underscore"); | ||
| const next_paren_index = comptime std.mem.indexOfScalar(u8, fq_name[start_index..], ')') orelse @compileError("Expected a paren"); | ||
| return @ptrCast(fq_name[(start_index + underscore_index + 1) .. start_index + next_paren_index] ++ [_]u8{0}); |
There was a problem hiding this comment.
The current implementation of fnName relies on parsing the output of @typeName(@TypeOf(func)). This is brittle as it depends on the compiler's internal representation of type names, which is not guaranteed to be stable across compiler versions. This goes against the design goal of safety and maintainability. Additionally, several lines in this function exceed the 100-column limit specified in the style guide.
A more robust and readable approach is to use the @fnName builtin, which directly provides the function's name. This simplifies the code, removes the dependency on compiler-internal implementation details, and helps adhere to the line length limit.
const full_name = @fnName(func);
const underscore_index = comptime std.mem.indexOfScalar(u8, full_name, '_') orelse @compileError("Expected an underscore in function name '" ++ full_name ++ "'");
return full_name[underscore_index + 1 ..] ++ "\x00";
References
- The design goals are safety, performance, and developer experience. Relying on unstable, internal compiler details for core functionality goes against the goals of safety and good developer experience. (link)
- Line length should be at most 100 columns. Lines 10, 11, and 12 in the current implementation exceed this limit. (link)
1 participant
No description provided.