-
Notifications
You must be signed in to change notification settings - Fork 7
internal/dirent: use binary.NativeEndian for parsing ints #63
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
Conversation
This change removes the platform specific integer parsing logic from internal/dirent and replaces it with binary.NativeEndian which is well tested and removes the need for us to maintain platform specific code. Fixes: #61
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #63 +/- ##
==========================================
- Coverage 60.19% 59.20% -1.00%
==========================================
Files 14 12 -2
Lines 824 804 -20
==========================================
- Hits 496 476 -20
Misses 314 314
Partials 14 14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
QuLogic
left a 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.
This did fix the test for me: https://koji.fedoraproject.org/koji/taskinfo?taskID=136601642
|
Ah, unfortunately, the last commit pushed after I tested doesn't pass now. |
| binary.NativeEndian.PutUint64(buf[:], want) | ||
| got, ok := readInt(buf, 0, uintptr(sz)) |
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.
This is going to be the wrong spot; on big-endian, the PutUint64 will write non-zero to the ending bytes but the read is on the starting bytes.
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.
Ah, thank you for pointing this out - will fix.
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.
This change removes the platform specific integer parsing logic from internal/dirent and replaces it with
binary.NativeEndianwhich is well tested and removes the need for us to maintain platform specific code. Additionally, it sets the minimum supported Go version to1.21which is required to usebinary.NativeEndian.Fixes: #61