sedlex
sedlex copied to clipboard
ocaml-community
Buffer channel input, return None at the end of partial reads.
Finally came up with the proper fix for #45 !
This PR adds proper buffering on channel reads while also inserting a None entry at the end of each partial read. This allow the fill_buf_from_gen function to return without reaching blocking read on the buffer.
It is probably only necessary to be careful about blocking on interactive uses; for non-interactive uses it probably slows things down a bit (and that can be significant when compiling a large code base; often lexing is one of the most time-consuming steps). I wonder if we should have two separate functions for this. Several other lexical analyzer generators have specific options for interactive vs. non-interactive use. Perhaps @Drup has an opinion.
A separate function could work too. I'm already doing that in the app itself.
@pmetzger I'm not so convinced that having two modes is important. We would need benchmarks to really make a decision.
Thanks for bringing this one up!
We have used this function in our application for a while and it's working pretty well. I say we could either merge it as it is or, if we can to be conservative with features, add those changes as a from_interactive_channel.
Do you think you could do a quick benchmark to see if we should bother with a distinct mode?
Running a quick benchmark, it seems that this PR is actually faster than the implementation on master, probably due to the fact that we perform less c calls.
That said, I cannot convince myself that the implementation is correct. In particular, I don't understand how we can be sure that we don't return None at the very start of the refill call (which would look like an EOF)
Also, I think we would have invalid results if a give-back-control None was returned in the middle of Utf8.Helper.from_gen.
Indeed! @hhugo do you have other pending changes? I'd say we should do a release soon now.
I don't have other changes planned. I wanted to run some more tests with jsoo first maybe.