-
Notifications
You must be signed in to change notification settings - Fork 141
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
Remove code duplication #618
base: master
Are you sure you want to change the base?
Conversation
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.
Seems largely fine with me, but I don't have time to compare Core. @sol could you please run benchmarks before and after? Ideally using https://github.com/Bodigrim/tasty-bench/blob/master/compare_benches.sh.
unsafeDrop, | ||
unsafeDropEnd, |
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 module is publicly exposed, so please update changelog and provide @since
annotations.
Use `unsafeDrop` and friends instead of explicitly constructing values with `Data.ByteString.Internal.BS`. - All those primitives are marked with `INLINE`. - This does not change the generated core.
|
Use
unsafeDrop
and friends instead of explicitly constructing values withData.ByteString.Internal.BS
.INLINE
.Underlying motivation:
Zero-copy conversion from
ByteString
toText
and O(1) conversion fromText
toByteString
offer substantial benefits. Despite #193 being closed, I still think it's desirable that the code makes it easier to experiment with different underlying representations. If at the same time, this reduces code duplication and makes the code easier to read, then the better.I only looked at construction sites for now. There are more low hanging fruit, but before tackling those I want to make sure that we are on the same page.