jonhoo commented 4 years ago

This patchset builds on #21, and bumps the dependency on nom to version 5. The change is a fairly major one, since nom moved away from macro-based parsers to combinators based on function callbacks.

Fixes #19

Old obsolete text:

The current state of affairs is that this almost compiles. The biggest remaining issue is how to pass the methods of expr::PRef in the new function-passing style. The errors should pretty quickly make it obvious what's going on. I'm honestly not sure what the right way to go about this is. The methods that call them may need to be re-written in the sort of "linear" style that nom 5 favors. nom-methods may provide some inspiration here. The new &mut self style also makes it awkward to have a combinator that may call multiple different methods on self. It may be that we want to return to the old "consume self return Self" strategy to get around this.

I don't know that I'll have the time to finish this PR up any time soon, so @jethrogb if you want to pick this up and run with it, feel free!

This change is

jonhoo commented 4 years ago

@Geal I noticed your thumbs-up :p Do you have any good suggestions for how to deal with parser methods that take &mut self combined with alt?

jonhoo commented 4 years ago

Apparently (?) the mut self wasn't necessary... I just turned them all into &self (which allows aliasing across the closures in the branches of any, and it just worked.. And all the tests pass. So I guess ready for review!

jonhoo commented 4 years ago

As a separate observation, since this will be a breaking change, I think we should also take the time to consider any other breaking changes we want to make to make to the API. Maybe @emilio has some ideas given the experience with cexpr in bindgen?

jonhoo commented 4 years ago

I can also happily report that I tried running the bindgen test suite with this branch of cexpr patched in, and all the tests pass with no further modifications! See https://github.com/rust-lang/rust-bindgen/pull/1735

jrmuizel commented 4 years ago

Is this ready to be merged?

jonhoo commented 4 years ago

@jrmuizel It should be -- just waiting on @jethrogb to review.

jonhoo commented 4 years ago

Should be all good now. I also went through and fixed all the clippy lints (which did change some signatures), and I added (minimal) documentation to things that did not have it that you may want to look over. For the future, I also added #[warn(missing_docs)].

jethrogb commented 4 years ago

bors r+

bors[bot] commented 4 years ago

Build succeeded

continuous-integration/travis-ci/push

jonhoo commented 4 years ago

I'll update https://github.com/rust-lang/rust-bindgen/pull/1735 once 0.4.0 is released to crates.io :)

jethrogb commented 4 years ago

Done

jethrogb / rust-cexpr

Upgrade to nom 5 #22

Build succeeded