-
Notifications
You must be signed in to change notification settings - Fork 228
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
[keccak-hash] Add no_std support, migrate to 2018 edition #177
Conversation
Migrate keccak-hash to 2018 edition; Replace H256 with [u8; 32] Signed-off-by: koushiro <koushiro.cqx@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the return type here and disentangle the crate from ethereum-types
has massive consequences for consuming code. It is going to take us a long while to go through and update all the rest. I think it would be a good idea to revert that change and add a new PR with just that change (alternatively go through parity-ethereum
and all other projects that use the crate and make sure the necessary changes are made).
Until then I'll put this PR on ice.
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Updated, PTAL again @dvdplm |
@dvdplm could you take a look again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question and I still don't see the point of no_std
tests; to me it seems like adding code for code's own sake rather than for a purpose.
keccak-hash/src/lib.rs
Outdated
use std::io; | ||
use std::slice; | ||
#[cfg(not(feature = "std"))] | ||
extern crate alloc; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't use alloc;
work here?
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@debris can I spend a review coin here? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still see no point in requiring tests to be no_std
.
Signed-off-by: koushiro koushiro.cqx@gmail.com
What have I changed
Add no_std support;
Migrate keccak-hash to 2018 edition;
related issue: #143