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

Use Vec to implement the Events iterators #117

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

cuviper
Copy link
Contributor

@cuviper cuviper commented Apr 22, 2020

Using vec::IntoIter is much simpler than a deeply nested Chain,
compiling faster and avoiding the deeper recursion limit reported in
rust#71359.

Using `vec::IntoIter` is much simpler than a deeply nested `Chain`,
compiling faster and avoiding the deeper recursion limit reported in
[rust#71359](rust-lang/rust#71359).
@cuviper
Copy link
Contributor Author

cuviper commented Apr 22, 2020

cc @chrysn #115

I'm not familiar with actually using this crate, as I just looked into this for the Rust issue. I'd be interested to know how this affects performance -- I hope the added cost of Vec allocation is offset by the much greater simplicity of the iterators.

@chrysn
Copy link
Contributor

chrysn commented Apr 24, 2020

In the test I've crudely sketched up, this behaves equivalently to the #115 version (which solves the same issue but just ups the recursion depth). A rough comparison indicates equal or slightly-better performance of this over #115, by I have zero trust in the test setup as I don't really know what I'm benchmarking or what for.

I really can't say anything about the suitability of the Vec allocation, as I don't yet use the parts of typed-html that deal with exotic allocators (like dodrio on bumpalo) where it may or may not be relevant.

@bodil
Copy link
Owner

bodil commented Apr 24, 2020

The Vec shouldn't have an impact on Dodrio, it couldn't be allocated using a bump allocator in any case.

Anyway, good work, merging 👍

@bodil bodil merged commit d95ce1a into bodil:master Apr 24, 2020
@bodil bodil mentioned this pull request Apr 24, 2020
@cuviper
Copy link
Contributor Author

cuviper commented May 22, 2020

@bodil could you publish a new release, so we might feel safer in closing that Rust issue? 🙂

@andybalaam
Copy link

Because there is no release yet, I depended directly on the latest git commit in my Cargo.toml like this:

typed-html = { git = "https://github.com/bodil/typed-html#4c13ecca" }

and my broken compile is fixed!

To feed the search engines, the compile error I had before was:

error: overflow representing the type `std::option::Option<&T>`

rofrol added a commit to rofrol/rustommerce that referenced this pull request Aug 24, 2020
iamcodemaker added a commit to iamcodemaker/euca that referenced this pull request Aug 31, 2020
This contains a fix for that now allows release builds to be made.

bodil/typed-html#117
@dullbananas
Copy link

@bodil you should release this fix to crates.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants