search

martinohmann / hcl-rs

HCL parsing and encoding libraries for rust with serde support
Apache License 2.0
122 stars 13 forks source link

deps: upgrade winnow to 0.5.0 #269

Closed martinohmann closed 1 year ago

epage commented 1 year ago

How did the new error type work out for you? Any additional thoughts from the upgrade?

martinohmann commented 1 year ago

How did the new error type work out for you? Any additional thoughts from the upgrade?

The new error type is great! I don't miss anything compared to what I had before. Decoupling ContextError from the input was a really great idea as it yields a nice performance improvement and also makes function signatures much cleaner.

Before I was forced to litter the codebase with lifetime annotations because my own error type was generic over the input (and PResult isn't, in contrast to IResult):

pub(super) fn string<'a>(input: &mut Input<'a>) -> PResult<String, ContextError<Input<'a>>> { ... }

Now this is just:

pub(super) fn string(input: &mut Input) -> PResult<String> { ... }

The only thing i did was overriding the error formatting to stick to the output of my previous error type, but you made that easy due to the provided context() and cause() methods on ContextError: https://github.com/martinohmann/hcl-rs/blob/5ca255f4d1404c19702727de9cdd191b86b5c0e6/crates/hcl-edit/src/parser/error.rs#L153-L205

I don't think this change needs to be incorporated into ContextError's Display implementation since error formatting is a matter of taste I think and the user should decide how to do it. The default implementation is reasonable. The only thing that we could incorporate is to emit "unexpected token" (like I did) when there's no expectation found in the context, but that's just a minor thing.

martinohmann commented 1 year ago

Overall, with the mutable streams and improved error type I'm seeing 15-20% performance improvement on linux and even ~30% on macos, which is great I think. Good job!