Tweak documentation #523
Tweak documentation #523
Conversation
Data/ByteString.hs
Outdated
| -- There are zero reasons to use 'head': it is both partial and slow. | ||
| -- Please consider using either 'uncons', which is total, or, if you are | ||
| -- adamant that the argument is non-empty, 'Data.ByteString.Unsafe.unsafeHead', | ||
| -- which is branchless and fast. | ||
| head :: HasCallStack => ByteString -> Word8 |
There was a problem hiding this comment.
I think it's good to reference uncons and unsafeHead here, but I disagree with the "zero resons to use head" bit. In certain situations head might still be the shortest / most ergonomic choice, and with its HasCallStack constraint it should be pretty easy to debug. unsafeHead may produce a segmentation fault – a risk that some users probably don't want to take on.
(Dito for last, tail, init and index.)
Data/ByteString/Lazy.hs
Outdated
| @@ -342,12 +342,14 @@ snoc cs w = foldrChunks Chunk (singleton w) cs | |||
| {-# INLINE snoc #-} | |||
|
|
|||
| -- | /O(1)/ Extract the first element of a ByteString, which must be non-empty. | |||
| -- | |||
| -- This is a partial function, please consider using 'uncons' instead. | |||
There was a problem hiding this comment.
| -- This is a partial function, please consider using 'uncons' instead. | |
| -- This is a partial function. Please consider using 'uncons' instead. |
Maybe even drop the "Please" – to me it sounds like the bytestring maintainers would benefit personally if users would use uncons instead.
(Dito for the other changes in this module.)
|
@sjakobi thanks, updated. |
Data/ByteString.hs
Outdated
| -- This function is both partial and slow. | ||
| -- Please consider using either 'uncons', which is total, or, if you are | ||
| -- adamant that the argument is non-empty, 'Data.ByteString.Unsafe.unsafeHead', | ||
| -- which is branchless and fast. |
There was a problem hiding this comment.
This reads like we want users to pick either uncons or unsafeHead instead of head. Is this your goal?
How about something like this:
| -- This function is both partial and slow. | |
| -- Please consider using either 'uncons', which is total, or, if you are | |
| -- adamant that the argument is non-empty, 'Data.ByteString.Unsafe.unsafeHead', | |
| -- which is branchless and fast. | |
| -- Related functions: | |
| -- | |
| -- * 'uncons', for safe access to the 'head' and 'tail' of a ByteString | |
| -- * 'Data.ByteString.Unsafe.unsafeHead' |
There was a problem hiding this comment.
Is this your goal?
To be honest yes, I'd like to see things rolling away from head. What's your take about it?
There was a problem hiding this comment.
I think users ought to be careful with head. But there still are legitimate use cases for it, where the code using head is preceded by a length check on the bytestring.
So I'm fine with warning about the partiality, and pointing out the alternatives.
But I don't want to push users to use unsafeHead rather than head. I think it's too dangerous to be used outside of the most performance-critical code.
There was a problem hiding this comment.
OK, let's make it less controversial. Updated.
(cherry picked from commit 2b9416d)
(cherry picked from commit 2b9416d)
No description provided.