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

Update nom #214

Merged
merged 6 commits into from
Mar 17, 2023
Merged

Update nom #214

merged 6 commits into from
Mar 17, 2023

Conversation

Skgland
Copy link
Contributor

@Skgland Skgland commented Mar 16, 2023

Upgrade from nom 4 to nom 7.

Done from scratch rather than using #140 as the rebase was ugly.

TODO

  • cleanup some functions
  • cleanup commits / commit messages

@dignifiedquire
Copy link
Member

Very cool, looking forward to reviewing this. Please let me know if you need any help.

@Skgland Skgland marked this pull request as ready for review March 17, 2023 21:17
@Skgland
Copy link
Contributor Author

Skgland commented Mar 17, 2023

@dignifiedquire should now be ready for review

src/types/mpi.rs Outdated Show resolved Hide resolved
src/types/mpi.rs Outdated Show resolved Hide resolved
@dignifiedquire
Copy link
Member

Looking good, just a clippy issue & the FIXMEs I believe

I checked nom 4 and for slices at_eof was always false

https://docs.rs/nom/4.0.0/src/nom/traits.rs.html#988-992
@dignifiedquire dignifiedquire mentioned this pull request Mar 17, 2023
3 tasks
@Skgland
Copy link
Contributor Author

Skgland commented Mar 17, 2023

I will fix rustfmt together with the fixme once I have an anser regarding that

@dignifiedquire
Copy link
Member

All manual tests also pass, including no regression in benchmarks.

@Skgland
Copy link
Contributor Author

Skgland commented Mar 17, 2023

  • last of my FIXMEs removed
  • rustfmt
  • clippy
    should be good now

@dignifiedquire dignifiedquire mentioned this pull request Mar 17, 2023
9 tasks
@dignifiedquire dignifiedquire merged commit be4eaaa into rpgp:master Mar 17, 2023
@dignifiedquire
Copy link
Member

Thanks again!

@Skgland Skgland mentioned this pull request Mar 17, 2023
@Skgland Skgland deleted the update-nom branch March 18, 2023 00:00
dignifiedquire pushed a commit that referenced this pull request Mar 18, 2023
* return a couple lost comments to where they belong

they apparently got lost on their way from nom 4 to nom 7

* repace `map(parser, |_| const)` with `value(const, parser)`

* replace some open coding with combinators
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.

2 participants