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

[keccak-hash] Add no_std support, migrate to 2018 edition #177

Merged
merged 5 commits into from
Aug 12, 2019
Merged

[keccak-hash] Add no_std support, migrate to 2018 edition #177

merged 5 commits into from
Aug 12, 2019

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Jun 4, 2019

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

Migrate keccak-hash to 2018 edition;

Replace H256 with [u8; 32]

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
dvdplm
dvdplm previously requested changes Jun 6, 2019
Copy link
Contributor

@dvdplm dvdplm left a 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>
@koushiro koushiro changed the title [keccak-hash] Add no_std support; Migrate to 2018 edition; Replace H256 with [u8; 32] [keccak-hash] Add no_std support, migrate to 2018 edition Jun 6, 2019
@koushiro
Copy link
Contributor Author

koushiro commented Jun 6, 2019

Updated, PTAL again @dvdplm

@ordian
Copy link
Member

ordian commented Jun 25, 2019

@dvdplm could you take a look again?

Copy link
Contributor

@dvdplm dvdplm left a 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.

use std::io;
use std::slice;
#[cfg(not(feature = "std"))]
extern crate alloc;
Copy link
Contributor

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?

koushiro and others added 2 commits June 26, 2019 10:17
@ordian ordian requested a review from debris July 16, 2019 13:02
@ordian
Copy link
Member

ordian commented Jul 16, 2019

@debris can I spend a review coin here? :)

Copy link
Contributor

@dvdplm dvdplm left a 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.

@dvdplm dvdplm merged commit cb94b50 into paritytech:master Aug 12, 2019
@koushiro koushiro deleted the update-keccak-hash branch August 13, 2019 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants