refactor: modernize Go syntax with range over integers#17
Open
AWaterColorPen wants to merge 3 commits intomainfrom
Open
refactor: modernize Go syntax with range over integers#17AWaterColorPen wants to merge 3 commits intomainfrom
AWaterColorPen wants to merge 3 commits intomainfrom
Conversation
- Replace traditional for loops with Go 1.22+ range over integers syntax - Updated files: api/models/graph.go, dictionary_splitter.go, dependency_graph.go, dictionary_adapter.go, dictionary_column.go - Improves code readability and aligns with modern Go best practices
…S bug The BFS/topological-sort loops in: - api/models/graph.go (GetTree) - dependency_graph.go (sort) - dictionary_adapter.go (GetDependencyTree) grow their queues inside the loop body, so 'for i := range len(queue)' is incorrect (range evaluates len once at entry). Restore these to 'for i := 0; i < len(queue); i++' which re-evaluates len each iteration. Static-length loops in dictionary_column.go and dictionary_splitter.go are safe to keep as 'for i := range len(...)'. All tests pass.
The Go modernization syntax PR uses 'for i := range len(...)' (Go 1.22+), but the CI workflows still specified go-version: 1.18, causing build failures. Update both go.yml and clickhouse.yml to use Go 1.24 to match go.mod.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR modernizes Go syntax by replacing traditional for loops with Go 1.22+ range over integers syntax, where safe to do so.
Changes
dictionary_column.goanddictionary_splitter.goto usefor i := range len(slice)syntaxfor i := 0; i < len(queue); i++form for BFS/topological-sort loops that dynamically grow their queue inside the loop bodyBug Fix (added in follow-up commit)
The original commit incorrectly applied
for i := range len(queue)to three dynamic-queue loops:api/models/graph.go—GetTree(BFS, queue grows in loop)dependency_graph.go—sort(topological sort, queue grows in loop)dictionary_adapter.go—GetDependencyTree(BFS, queue grows in loop)range len(queue)evaluateslen(queue)once at entry, so it cannot traverse elements appended during iteration. These have been reverted to the classic form.Why range-len is safe for the other two
dictionary_column.go— iterates a fixed-length sorted slice, no mutationdictionary_splitter.go— iteratesdl1which is not modified in the loop bodyTesting
Related to Phase 1 of olap-sql modernization plan
This is part of the Phase 1 task: "使用新 Go 语法重构" (Use new Go syntax refactoring).