-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Reset export renamer count to 0 at end of each chunk #22763
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughReset of the ExportRenamer internal counter in Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. Comment |
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 (3)
src/ast.zig (3)
158-161
: Solid switch to fixed-size, 64B-aligned tables; add compile-time guards to prevent length drift.The change looks good. To harden against accidental mismatches (including future edits or CharFreq changes), add comptime checks tying these constants to their consumers.
Apply this diff right after the two consts:
pub const default_head: [54]u8 align(64) = "etnriaoscludfpmhg_vybxSCwTEDOkAjMNPFILRzBVHUWGKqJYXZQ$".*; pub const default_tail: [64]u8 align(64) = "etnriaoscludfpmhg_vybxSCwTEDOkAjMNPFILRzBVHUWGKqJYXZQ$1024368579".*; + +comptime { + if (default_head.len != 54) @compileError("NameMinifier.default_head must be 54 bytes"); + if (default_tail.len != char_freq_count) + @compileError("NameMinifier.default_tail length must equal char_freq_count"); +}Please confirm this compiles cleanly with the Zig version pinned in the repo, given the sentinel-to-non-sentinel coercion from the string literal via
.*
.
184-199
: PrefertoOwnedSlice()
to transfer ownership explicitly.Returning
name.items
relies on callers freeing a slice that originated from an internal ArrayList buffer. UsingtoOwnedSlice()
makes ownership explicit and avoids accidental leaks if the calling pattern changes.Apply this diff:
- return name.items; + return name.toOwnedSlice();If there are callers relying on the current behavior/capacity, please double-check them. If throughput is a concern, we can also pre-size
name
by estimating digits fromi
to minimize reallocs.
162-167
: Offer a convenience initializer that seeds head/tail with defaults.
numberToMinifiedName
assumesthis.head
/this.tail
are populated. A helper reduces footguns and branches callers onto the default path.Add alongside
init
:pub fn init(allocator: std.mem.Allocator) NameMinifier { return .{ .head = std.ArrayList(u8).init(allocator), .tail = std.ArrayList(u8).init(allocator), }; } + + pub fn initWithDefaults(allocator: std.mem.Allocator) !NameMinifier { + var nm = NameMinifier.init(allocator); + try nm.head.appendSlice(default_head[0..]); + try nm.tail.appendSlice(default_tail[0..]); + return nm; + }If a non-default charset is ever required, this keeps the existing
init
path intact while making the default the easy path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/ast.zig
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Implement debug logs in Zig using
const log = bun.Output.scoped(.${SCOPE}, false);
and invokinglog("...", .{})
Files:
src/ast.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig
: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
**/*.zig
: Format Zig files with zig-format (bun run zig-format)
In Zig, manage memory carefully with allocators and use defer for cleanup
Files:
src/ast.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). (1)
- GitHub Check: Format
What does this PR do?
How did you verify your code works?