-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add AppendInt function with positive/negative number convertion to string #90
Conversation
…ring Benchmark_ItoA/fiber (pos_num)-12 190428255 6.13 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (pos_num)-12 196626811 6.08 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80716807 14.33 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80445802 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81137728 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81345360 14.49 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/fiber (neg_num)-12 151121002 7.81 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (neg_num)-12 153566410 7.70 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 84505304 13.75 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 82524801 13.76 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 84884136 13.88 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 85829492 13.76 ns/op 5 B/op 1 allocs/op
…ring Benchmark_ItoA/fiber (pos_num)-12 190428255 6.13 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (pos_num)-12 196626811 6.08 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80716807 14.33 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80445802 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81137728 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81345360 14.49 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/fiber (neg_num)-12 151121002 7.81 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (neg_num)-12 153566410 7.70 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 84505304 13.75 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 82524801 13.76 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 84884136 13.88 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 85829492 13.76 ns/op 5 B/op 1 allocs/op
…ring Benchmark_ItoA/fiber (pos_num)-12 190428255 6.13 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (pos_num)-12 196626811 6.08 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80716807 14.33 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80445802 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81137728 14.30 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81345360 14.49 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/fiber (neg_num)-12 151121002 7.81 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (neg_num)-12 153566410 7.70 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 84505304 13.75 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 82524801 13.76 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 84884136 13.88 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 85829492 13.76 ns/op 5 B/op 1 allocs/op
Caution Review failedThe pull request is closed. WalkthroughThe recent updates enhance integer-to-string conversion capabilities by introducing the new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #90 +/- ##
==========================================
+ Coverage 92.10% 92.48% +0.37%
==========================================
Files 9 9
Lines 380 399 +19
==========================================
+ Hits 350 369 +19
Misses 17 17
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
go.mod
is excluded by!**/*.mod
go.sum
is excluded by!**/*.sum
,!**/*.sum
Files selected for processing (3)
- README.md (3 hunks)
- convert.go (2 hunks)
- convert_test.go (3 hunks)
Additional comments not posted (5)
convert.go (2)
14-15
: Import statement forfasthttp
package.The
fasthttp
package is imported to facilitate theAppendInt
function.
157-171
: LGTM! But verify the usage offasthttp.AppendUint
.The code changes are approved.
However, ensure that
fasthttp.AppendUint
handles the conversion as expected.README.md (1)
63-74
: Comprehensive benchmark tests forBenchmark_ItoA
.The benchmark tests are well-structured and provide valuable performance metrics for integer-to-string conversion methods.
convert_test.go (2)
236-247
: Comprehensive test cases forAppendInt
.The test cases cover a wide range of inputs, ensuring comprehensive validation of the
AppendInt
function.
313-378
: Comprehensive benchmark tests forBenchmark_ItoA
.The benchmark tests are well-structured and provide valuable performance metrics for integer-to-string conversion methods.
…ring Benchmark_ItoA/fiber (pos_num)-12 186969812 6.25 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (pos_num)-12 193965686 6.16 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80716807 14.42 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (pos_num)-12 80445802 14.85 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81137728 14.52 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt_(pos_num)-12 81345360 14.51 ns/op 4 B/op 1 allocs/op Benchmark_ItoA/fiber (neg_num)-12 192902048 6.18 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/fiber (neg_num)-12 194245189 6.14 ns/op 0 B/op 0 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 84505304 13.55 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.Itoa (neg_num)-12 82524801 13.54 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 84884136 13.63 ns/op 5 B/op 1 allocs/op Benchmark_ItoA/strconv.FormatInt (neg_num)-12 85829492 13.63 ns/op 5 B/op 1 allocs/op
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: 2
Outside diff range, codebase verification and nitpick comments (2)
convert.go (2)
163-174
: Performance: Optimize buffer usage.The buffer size of 20 bytes is sufficient for 64-bit integers, but consider documenting this assumption for clarity.
// Buffer size of 20 bytes is sufficient for 64-bit integers var b [20]byte
176-179
: Readability: Improve comment clarity.The comment can be more descriptive to explain the purpose of adding the '-' sign.
if isNegative { // Prepend '-' to indicate a negative number dst = append(dst, '-') }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- README.md (3 hunks)
- convert.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- README.md
func AppendInt(dst []byte, n int) []byte { | ||
isNegative := n < 0 | ||
if isNegative { | ||
// Convert the number to positive | ||
n = -n | ||
} |
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.
Correctness: Handle integer overflow for negative values.
When converting a negative integer to positive, be cautious of integer overflow. The edge case for n = math.MinInt
should be handled.
if isNegative {
if n == math.MinInt {
// Handle the edge case for the smallest possible integer
dst = append(dst, "-9223372036854775808"...)
return dst
}
n = -n
}
@@ -151,3 +151,34 @@ func ToString(arg any, timeFormat ...string) string { | |||
return fmt.Sprint(arg) | |||
} | |||
} | |||
|
|||
// AppendInt appends the string representation of the int n to dst and returns the extended buffer. | |||
func AppendInt(dst []byte, n int) []byte { |
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.
I think we need a separate function for AppendUint
that uses uint
as param, else we won't be able to handle values bigger than 2,147,483,647
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.
The math
module in Golang defines these:
MaxUint32 = 1<<32 - 1 // 4294967295
MaxUint64 = 1<<64 - 1 // 18446744073709551615
convert_test.go
Outdated
require.Equal(b, []byte(numberString), resB) | ||
}) | ||
|
||
b.Run("default - strconv.Itoa (positiv number)", func(b *testing.B) { |
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.
positive*
convert_test.go
Outdated
require.Equal(b, numberString, resS) | ||
}) | ||
|
||
b.Run("default - strconv.FormatInt (positiv number)", func(b *testing.B) { |
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.
positive*
there is already a AppendInt function in strconv |
Summary by CodeRabbit
New Features
Bug Fixes
Tests