search

haskell-streaming / streaming-bytestring

effectful sequences of bytes; an alternative no-lazy-io implementation of Data.ByteString.Lazy
BSD 3-Clause "New" or "Revised" License
16 stars 11 forks source link

Better `readInt` take two #31

Closed vdukhovni closed 3 years ago

vdukhovni commented 3 years ago

This version still outperforms the original by ~4-5x, and now implements correct overflow checks for the full range of Int values.

A "reasonable" number of leading zeros is supported after an optional explicit sign, but once ~32k of leading zeros have been seen, no further chunks will be processed and the read fails (returns Nothing as the Maybe Int payload).

It also adds skipSomeWS which skips a "reasonable" quantity of leading whitespace before giving up if more than ~32k of whitespace is encountered.

The new w8IsSpace function is somewhat out of place here, but it is more efficient than any stock versions I know of in any other module (barring writing some FFI code for this). It could be kept internal, with folks who want to roll their own skipping of leading whitespace also providing their own implementation of the predicate...

[ EDIT: got rid of the readIntSkip function, turns out just providing skipSomeWS separately actually works just as well, with no need to provide the composition. ]

vdukhovni commented 3 years ago

Squashed the fixup (older Preludes don't have (<>) built-in). So the CI should now be green.

So please let me know which of #29 or #31 (this PR) you'd like to progress.

CC: @chessai, @hsyl20, @bodigrim, @sjakobi, @cartazio

sjakobi commented 3 years ago

@vdukhovni Would you mind pinging me less often about things related to streaming-bytestring? I'm currently not very interested in this package. Thanks! :)

vdukhovni commented 3 years ago

@vdukhovni Would you mind pinging me less often about things related to streaming-bytestring? I'm currently not very interested in this package. Thanks! :)

Sure (sorry about a final notification this will cause). The reason I thought this is relevant is that there's an open discussion re readInt wrt. bytestring and overflow detection, ... so I thought there'd be some relevance. Apologies. Will honour your request going forward...

vdukhovni commented 3 years ago

Many thanks! Just pushed a squashed version.

vdukhovni commented 3 years ago

Many thanks! Just pushed a squashed version.

It is the same as what you reviewed, just squashed into one commit. So I think this is ready to be merged...

vdukhovni commented 3 years ago

Thanks!

fosskers commented 3 years ago

Prepping for release soon, once I get mine merged in too.