search

LeopoldArkham / Molten

[WIP] Molten - Style-preserving TOML parser.
Apache License 2.0
40 stars 8 forks source link

WIP: Remove string copying. #15

Closed ehuss closed 6 years ago

ehuss commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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 commented 6 years ago

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 !