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

WIP: Remove string copying. #15

Merged
merged 17 commits into from
Nov 16, 2017

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Oct 21, 2017

This is an attempt to see what things look like if it uses string references instead of copying. This is incomplete, but all of the tests now pass. The Index API is broken, but should be fixable.

Overall it should be fairly straightforward, it just requires a lot of lifetime annotations. I had to remove the backtracking (and some of the lookaheads), which is probably the biggest part of this change.

If you like this idea, I can continue working on this cleaning things up and working on edge cases. There are a few places that need to be Cow to support modifying the value, but that is easy to add.

@LeopoldArkham
Copy link
Owner

LeopoldArkham commented Oct 23, 2017

Hey, sorry it took a while to get to this.

Since it involves heavy changes in the parser, I'm going to try to write new tests to make sure everything is represented correctly internally ahead of merging. The current reproduction tests don't ensure nesting is correct for instance.

Meanwhile you have time to keep cleaning up.

Thanks!

@ehuss
Copy link
Contributor Author

ehuss commented Oct 23, 2017

I can help set up more tests if you want. I was thinking it would be useful to bring in the semi-official test suite from https://github.com/BurntSushi/toml-test/ and some of the more up-to-date forks from https://github.com/uiri/toml-test and https://github.com/skystrife/toml-test. Let me know if you want a PR with that. In the future it might be nice to also use rust-fuzz.

@LeopoldArkham
Copy link
Owner

Yeah it'd be great to have all of those too.
Anyways, I'm gonna try pushing to your branch soon to help move this forwards but time is scarce right now.

@LeopoldArkham
Copy link
Owner

Whew!
So these changes had introduced a lot of regressions - this is my fault, the tests were insufficient to make sure the internal structure of the documents was correct.
This is all fixed now, and I added a second type of tests for the internals.

Some areas need cleanup but tests are green so I'm going to merge so there can be movement on other issues.

Thanks @ehuss !

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

2 participants