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.

Sign up for GitHub

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

Jump to bottom

Introduce the Engine abstraction. #157

Merged
merged 6 commits into from Aug 18, 2021
Merged

Introduce the Engine abstraction. #157

merged 6 commits into from Aug 18, 2021

Conversation

marshallpierce
Copy link
Owner

This will allow users to plug different implementations in to the rest of the API.

While I'm touching almost everything and breaking backwards compatibility, a bunch of renaming and cleanup was also done.

@marshallpierce
Introduce the Engine abstraction.
78619fb 
This will allow users to plug different implementations in to the rest of the API.

While I'm touching almost everything and breaking backwards compatibility, a bunch of renaming and cleanup was also done.
@marshallpierce
Copy link
Owner Author

@dingelish How does this look for creating a new Engine to house the logic from #153? I made a Naive implementation just for use in tests but it might be a good place to start from. It's smaller and simpler than the FastPortable one (which is just the previous logic repackaged).

@marshallpierce marshallpierce marked this pull request as ready for review February 14, 2021 16:08
@dingelish
Copy link

sure. i'd create another PR :-) thank you!

@marshallpierce marshallpierce mentioned this pull request Mar 8, 2021
Closed
@dingelish
Copy link

hey @marshallpierce how's it going! the security researchers have disclosed the CVE vulnerability
https://nvd.nist.gov/vuln/detail/CVE-2021-24117

@marshallpierce
Copy link
Owner Author

Yep, I saw that, and wondered if that was related. :) Did you get a chance to implement a constant-time Engine implementation?

@dingelish
Copy link

haven't start implementing Engine. but i'd be very happy to rebase this :-) hdyt?
https://github.com/dingelish/rust-base64/commits/master

@marshallpierce
Copy link
Owner Author

Let's get this done. I'll review this engine pr (perhaps some API changes will jump out at me after a few months' absence).

@marshallpierce marshallpierce force-pushed the mp/engine branch 3 times, most recently from 6fbe921 to 394a47a Compare August 6, 2021 20:11
@marshallpierce
Make constructing an Alphabet from a str const.
3c62bd4 
Not useful yet since unwrap() and friends aren't const, but some future rust version can make use of it.
@marshallpierce
Various cleanups
3155ca7 
- Improve engine tests
- Improve comments
- Remove dead code
- Improve error message byte formatting
@marshallpierce marshallpierce merged commit e3525bb into master Aug 18, 2021
@dingelish
Copy link

cool! i'll work on this tonight!

This was referenced Aug 18, 2021
Closed
Closed
@dingelish
Copy link

@marshallpierce should i create another Engine or add some feature gates on the current fast_portable engine?

@marshallpierce
Copy link
Owner Author

I think a new engine would be the way to go -- hopefully a simpler mechanism for users to choose from.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@marshallpierce @dingelish