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 to nom 3.0 #9

Merged
merged 3 commits into from May 12, 2017
Merged

Update to nom 3.0 #9

merged 3 commits into from May 12, 2017

Conversation

sdroege
Copy link
Contributor

@sdroege sdroege commented May 12, 2017

No changes needed

@sdroege
Copy link
Contributor Author

sdroege commented May 12, 2017

@Geal It might make sense to also move be_i24 to nom. If you think that's a good idea, I'll send a pull request your way

@Geal
Copy link
Collaborator

Geal commented May 12, 2017

that was fast :D

Sure, please send a PR for be_i24.

The CI does not pass because apparently, Rust 1.2 did not have overflowing_add accessible by default. I'm fine with removing 1.2 from the CI and only supporting from current stable.

1.2 does not have overflowing_add.
@sdroege
Copy link
Contributor Author

sdroege commented May 12, 2017

Ok, all done :)

@Geal
Copy link
Collaborator

Geal commented May 12, 2017

great!

@sdroege
Copy link
Contributor Author

sdroege commented May 12, 2017

Also found a bug in nom's le_u24 while at it.

Unrelated, I failed to talk to you at RustFest, but were you experimenting with moving nom to some "impl trait" approach instead of macros at some point? Does it make sense or is there still too much missing on the Rust side to make that a viable alternative?

@Geal Geal merged commit af0b9be into rust-av:master May 12, 2017
@Geal
Copy link
Collaborator

Geal commented May 12, 2017

I'm still experimenting, and I think it's much too soon to switch to an impl Trait solution. Basically, the unanswered questions right now:

  • does it increase or decrease compilation time?
  • does it increase or decrease running time?
  • can we have a seamless upgrade path where the code of macros is first replaced with impl trait stuff, then removed later?

But this is something I would have liked to do at the beginning of nom, so I'm planning to switch at some point.

@sdroege
Copy link
Contributor Author

sdroege commented May 12, 2017

Great, thanks :) And sure, those points all need to be answered and tested carefully. But even apart from that, before "impl trait" is stable there's no point in switching anyway. Might be a good time to start experiments though

@Geal
Copy link
Collaborator

Geal commented May 12, 2017

that's also a huge amount of work :)

@sdroege
Copy link
Contributor Author

sdroege commented May 12, 2017

Of course :) Someone seriously need to invent longer days ;)

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