refactor: remove init$patchDeps mechanism (no longer needed after PR #1450)#1596
Conversation
…oplus#1450) Since PR goplus#1450 changed ABI types to compile-time constant data, the init$after lazy loading mechanism no longer exists. This means the initialization order issue that PR goplus#1414 fixed is no longer possible: - Before PR goplus#1450: Types were initialized at runtime via init$after with lazy loading (check null → create). Cross-package initialization order could cause types to be skipped. - After PR goplus#1450: Types are compile-time constants embedded directly in LLVM IR. No runtime initialization, no lazy loading, no order issues. This commit removes: - init$patchDeps function generation - initDepsFnNameOfHasPatch() helper - findFirstNonInitInstruction() helper - createInitDepsFunction() method - Related .init$patchDeps handling in notInit() The overlay package init sequence is simplified back to: overlay.init() → call init$hasPatch() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary of ChangesHello @luoliwoshang, 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 refactors the compiler by eliminating the 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
|
There was a problem hiding this comment.
Code Review
This is an excellent refactoring that correctly removes the init$patchDeps mechanism, which became obsolete after the changes in PR #1450. The removal of createInitDepsFunction and its associated helper functions is clean and complete. The related logic in compileBlock and notInit has been simplified accordingly. These changes reduce code complexity and improve maintainability by eliminating unnecessary logic. The pull request is well-explained and the implementation aligns perfectly with the stated goal.
|
Code Review Summary This PR cleanly removes the
The PR description clearly explains the rationale. LGTM. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1596 +/- ##
==========================================
- Coverage 91.01% 91.00% -0.02%
==========================================
Files 45 45
Lines 11971 11927 -44
==========================================
- Hits 10896 10854 -42
+ Misses 899 898 -1
+ Partials 176 175 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Reopening this PR. After re-analysis: The If we remove this extraction:
The execution is equivalent but simpler: Before (with init$patchDeps): After (simplified): Same functionality, cleaner code! |
| @@ -508,15 +502,7 @@ func (p *context) compileBlock(b llssa.Builder, block *ssa.BasicBlock, n int, do | |||
| isCgoCmacro := isCgoCmacro(fnName) | |||
| for i, instr := range instrs { | |||
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package but no pkgFNoOldInit | |||
There was a problem hiding this comment.
Minor: The comment mentions pkgFNoOldInit but the condition doesn't check for it. Consider clarifying what the comment is documenting:
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package but no pkgFNoOldInit | |
| for i, instr := range instrs { | |
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package: call original init$hasPatch |
This would make it clearer that the comment describes the action being taken, not what's being checked.
| @@ -508,15 +502,7 @@ func (p *context) compileBlock(b llssa.Builder, block *ssa.BasicBlock, n int, do | |||
| isCgoCmacro := isCgoCmacro(fnName) | |||
| for i, instr := range instrs { | |||
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package but no pkgFNoOldInit | |||
There was a problem hiding this comment.
Minor: The comment mentions pkgFNoOldInit but the condition doesn't check for it. Consider clarifying what the comment is documenting:
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package but no pkgFNoOldInit | |
| for i, instr := range instrs { | |
| if i == 1 && doModInit && p.state == pkgInPatch { // in patch package: call original init$hasPatch |
This would make it clearer that the comment describes the action being taken, not what's being checked.
Code Review SummaryClean refactoring that removes dead code after PR #1450. The removal is complete and consistent - no orphaned references to Highlights:
One minor inline comment suggestion for comment clarity. LGTM otherwise. |
Summary
Since PR #1450 changed ABI types to compile-time constant data, the
init$afterlazy loading mechanism no longer exists. This means the initialization order issue that PR #1414 fixed is no longer possible.Before PR #1450
init$afterwith lazy loading (check null → create)init$patchDepsto ensure correct initialization orderAfter PR #1450
init$patchDepsmechanism is no longer neededLLVM IR Changes
Before (with
init$patchDeps)After (simplified)
Diff
define void @"go/build.init"() { store i1 true, ptr @"go/build.init$guard", align 1 - call void @"go/build.init$patchDeps"() call void @"go/build.init$hasPatch"() } -define void @"go/build.init$patchDeps"() { - call void @"bufio.init"() - call void @"bytes.init"() - ; ... - ret void -} define void @"go/build.init$hasPatch"() { + call void @"bufio.init"() ; dependency inits now stay here + call void @"bytes.init"() + ; ... ; global variable initialization }Code Changes
This PR removes:
init$patchDepsfunction generationinitDepsFnNameOfHasPatch()helperfindFirstNonInitInstruction()helpercreateInitDepsFunction()method.init$patchDepshandling innotInit()Related PRs
🤖 Generated with Claude Code