winnow-rs / winnow

Making parsing a breeze
https://docs.rs/winnow
Other
525 stars 40 forks source link

Switch impl Parser for u8 from one_of to tag(Simplify debug trace) #427

Closed baoyachi closed 7 months ago

baoyachi commented 8 months ago

Please complete the following tasks

rust version

rustc 1.74.0 (79e9716c9 2023-11-13)

winnow version

0.5.34

Minimal reproducible code

use winnow::combinator::preceded;
use winnow::prelude::*;
use winnow::error::InputError;
fn main() {

    fn parser<'s, 'a>(i: &mut &'s [u8]) -> PResult<u8, InputError<&'s [u8]>> {
        preceded(b"hello", b'a').parse_next(i)
    }
    assert_eq!(parser.parse_peek(&b"helloabc"[..]), Ok((&b"bc"[..], b'a')));
}

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

u8 originally usedone_of combinator, debugging is more verbose, and using tag combinator is much simpler.

Expected Behaviour

make debug output verbose simple

Additional Context

No response

epage commented 8 months ago

We have #418 for doing this for char but I guess we can keep this open to make sure we track u8 as well. It is a breaking change, so we have to wait for 0.6 as it requires changes to trait bounds

baoyachi commented 8 months ago

Yes, this is similar to #418, but there are many changes and modifications in #418. There are still some differences in the handling of char and u8, so in order to track pull requests, I still started a new issue

epage commented 8 months ago

Also, for me, performance is a higher priority for what the default is than -Fdebug so we'll need benches. It can be even or a barely noticeable drop but can't be noticeably slower.

epage commented 8 months ago

To be more explicit, the way to move this forward is to have the subset of what works in 0.5 split out and merged before the 0.6 work begins. I reserve breaking releases for only the changes that need it. For example, I'm taking care of deprecating some API changes now and only leaving the removal of deprecated APIs for 0.6.0, rather than doing it all at once.