Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom containers for many functions #122

Closed
2 tasks done
epage opened this issue Feb 3, 2023 · 1 comment · Fixed by #127
Closed
2 tasks done

Support custom containers for many functions #122

epage opened this issue Feb 3, 2023 · 1 comment · Fixed by #127
Labels
A-combinator Area: combinators C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@epage
Copy link
Collaborator

epage commented Feb 3, 2023

Please complete the following tasks

winnow version

0.2.0

Describe your use case

I wanted to experiment with porting toml_edit to nom, one thing I especially found missing was custom collection support. If I use many0, it only supports Vec. The naive approach in code is to do

fn ml_body(input: &str) -> IResult<&str, String> {
    many(substr).map(|s| s.join(""))
}

Leading me to allocate a Vec<String> and then merge them. combine defaults to Vec but allows any Extend type, allowing

fn ml_body(input: &str) -> IResult<&str, String> {
    many(substr)
}

which will collect directly into the String, saving on a lot of allocations

I'm also looking at optimizing a section of my parser where collect into a Vec and then turn it into a HashMap with errors by creating a custom type that wraps HashMap with my error type and stores off errors on Extend, allowing me to bypass the Vec allocation.

I could do all of this with fold_ variants but it will be a bit messier.

Describe the solution you'd like

Minimum

  • Supports common containers (Vec, String, etc)

Ideal

  • Supports all containers
  • Pre-allocates

One option I've been considering is

trait Foo {
    type Item;
    type Err;

    fn initial(capacity: Option<usize>) -> Result<Self, Self::Err>;

    fn push(&mut self, item: Self::Item) -> Result<(), Self::Err>;
}

(Err was added last minute to support #99)

We could implement this for

  • ()
  • usize
  • String
  • Vec
  • HashMap
  • BTreeMap
  • winnow::*::Extendable which wraps any Default + Extend

Questions

  • Name of trait
  • Where trait and Extendable live

Alternatives, if applicable

rust-bakery/nom#1621 solved this by supporting any Default + Extends container, losing pre-allocations

Additional Context

No response

@epage epage added A-combinator Area: combinators C-enhancement Category: Raise on the bar on expectations labels Feb 3, 2023
@epage epage added this to the 0.3 milestone Feb 3, 2023
@epage
Copy link
Collaborator Author

epage commented Feb 3, 2023

I wonder if this can replace the existing ExtendInto trait used for escapes. That instead is determined based on the input type and we'd be switching it to be based on the output type.

epage added a commit to epage/winnow that referenced this issue Feb 3, 2023
I went with a custom trait so we could cover `()` and `usize` as well as
support custom capacities.

This comes at the cost of supporting any container possible.  We could
possibly do that with a `Extendable(T: Default + Extend)` newtype.

As an alternative to `()` and `usize` is GATs.  The main difference is
who drives the types.

Fixes winnow-rs#122
epage added a commit to epage/winnow that referenced this issue Feb 3, 2023
I went with a custom trait so we could cover `()` and `usize` as well as
support custom capacities.

This comes at the cost of supporting any container possible.  We could
possibly do that with a `Extendable(T: Default + Extend)` newtype.

As an alternative to `()` and `usize` is GATs.  The main difference is
who drives the types.

Fixes winnow-rs#122
epage added a commit to epage/winnow that referenced this issue Feb 3, 2023
I went with a custom trait so we could cover `()` and `usize` as well as
support custom capacities.

This comes at the cost of supporting any container possible.  We could
possibly do that with a `Extendable(T: Default + Extend)` newtype.

As an alternative to `()` and `usize` is GATs.  The main difference is
who drives the types.

Fixes winnow-rs#122
@epage epage closed this as completed in #127 Feb 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-combinator Area: combinators C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant