Closed vdukhovni closed 3 years ago
This library is small, so I pretty much just speed read the whole thing, and tackled anything that caught my eye as looking a bit odd. Many of the functions are plain enough to not require any attention, but a few stood out...
We can consider the benchmarking issue in a later PR. For now, let's kill those "signature comments" and then merge this.
Oh, would you also mind updating the CHANGELOG?
We can consider the benchmarking issue in a later PR. For now, let's kill those "signature comments" and then merge this.
Pushed a commit that drop the module export list signature comments. I would however like to draw your attention to a couple of places where we still need to make decisions:
The fold
and fold_
signatures are unnatural
in the "Word8" module, as a breaking change, we should probably switch them around. Also the uncons
issues could be folded into this PR, or done separately...
Oh, would you also mind updating the CHANGELOG?
We can do that once the full scope is known. Still a few minor issues to decide...
Once this is almost ready, I'd like to squash the commits.
How are the fold
signatures unnatural?
How are the
fold
signatures unnatural?
Well, this was the one thing that side-by-side signatures in the export list were good for. :-)
For almost all the functions somefunc
is the one the retains the r
value in the result structure and somefunc_
is the one that drops it. However, with fold
in the non-Char8
module, it is the other way around:
$ git grep '^fold_* *:: ' | sed 's/:: Monad m => /:: /'
ByteString.hs:fold :: (x -> Word8 -> x) -> x -> (x -> b) -> ByteStream m () -> m b
ByteString.hs:fold_ :: (x -> Word8 -> x) -> x -> (x -> b) -> ByteStream m r -> m (Of b r)
ByteString/Char8.hs:fold_ :: (x -> Char -> x) -> x -> (x -> b) -> ByteStream m () -> m b
ByteString/Char8.hs:fold :: (x -> Char -> x) -> x -> (x -> b) -> ByteStream m r -> m (Of b r)
I think that while we're doing breaking change housekeeping, the first two should be switched.
I agree, let's move ahead with that.
I agree, let's move ahead with that.
I added this as a fixup commit and also including the toStrict_
signature change. Next on the agenda is uncons
. Any chance I can bundle that into #42 also?
Squashed and added CHANGELOG entries, almost ready to merge, modulo including the uncons
-related fixes in this PR.
Feel free to go ahead with all the uncons
fixes.
I made the uncons
changes as a separate unsquashed commit, should be easier to review. I think that's everything now...
Sweet, if all goes well I should be able to merge this tomorrow morning (it's 4pm here).
Thank you very much for this!
denull
. Less abstract machinery.SpecConstrAnnotation
.