Modernize pkg#12722
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Code Review
This pull request modernizes the codebase by adopting Go 1.21+ features, including replacing interface{} with any, utilizing for range loops, adopting slices.Contains and maps.Copy, and refactoring goroutine management with sync.WaitGroup helpers. Additionally, tests are updated to use t.Context(). The feedback suggests further optimizing the HexKey function in pkg/spanz/convert.go by using hex.EncodeToString and strings.ToUpper instead of a loop with fmt.Sprintf and strings.Builder.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func HexKey(key []byte) string { | ||
| // TODO(qupeng): improve the function. | ||
| str := "" | ||
| var str strings.Builder | ||
| for _, c := range key { | ||
| str += fmt.Sprintf("%02X", c) | ||
| str.WriteString(fmt.Sprintf("%02X", c)) | ||
| } | ||
| return str | ||
| return str.String() | ||
| } |
There was a problem hiding this comment.
Using fmt.Sprintf in a loop to convert a byte slice to a hex string is highly inefficient because it involves reflection, parsing the format string, and multiple allocations per byte. Since there is already a TODO to improve this function, we can implement it much more efficiently using the standard library's hex.EncodeToString and strings.ToUpper.
Note: You will need to import "encoding/hex" in this file.
func HexKey(key []byte) string {
// TODO(qupeng): improve the function.
return strings.ToUpper(hex.EncodeToString(key))
}
What problem does this PR solve?
Issue Number: ref #12691
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note