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

[rlp] Add no_std support #206

Merged
merged 11 commits into from
Sep 5, 2019
Merged

[rlp] Add no_std support #206

merged 11 commits into from
Sep 5, 2019

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Aug 29, 2019 •
edited by ordian

Signed-off-by: koushiro koushiro.cqx@gmail.com

What have I changed?

  • Add no_std support for rlp crate
  • Fix some warnings
  • Convert rlp benches to criterion

related issue: #167, closes #171

@koushiro
Add no_std support for rlp
8de5d15 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Remove hex-literal
b11a623 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

Sorry to reply so late. I will split #167 into multiple PRs. PTAL @dvdplm

@koushiro
Copy link
Contributor Author

koushiro commented Sep 3, 2019

Does anyone can review my code?

ordian
ordian approved these changes Sep 3, 2019
View reviewed changes
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

.travis.yml Outdated Show resolved Hide resolved
@ordian ordian requested a review from dvdplm September 4, 2019 11:04
@koushiro
Merge remote-tracking branch 'upstream/master' into rlp-no-std
507671a 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Fix some warnings
78e98da 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Remove std feature from primitive-types rlp feature
ef9d729 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
dvdplm
dvdplm reviewed Sep 5, 2019
View reviewed changes
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.

Overall it looks good.

We have this ongoing (implicit) discussion about rustc-hex. It is a low-priority long-term goal to remove it from all Parity projects so the changes here that re-introduces it are a problem. Can you elaborate on why you seem to dislike hex-literal and prefer rustc-hex?

@koushiro
Improve benchmark
2bd7d7c 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

koushiro commented Sep 5, 2019

Overall it looks good.

We have this ongoing (implicit) discussion about rustc-hex. It is a low-priority long-term goal to remove it from all Parity projects so the changes here that re-introduces it are a problem. Can you elaborate on why you seem to dislike hex-literal and prefer rustc-hex?

I just want to reduce the number of crates, if rustc-hex will be removed in the future, then I will revert it.

@koushiro
Revert hex-literal
2a86b5e 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Fix impl-rlp
65ef685 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

koushiro commented Sep 5, 2019

@dvdplm I want to know if you will release a new version for rlp crate on crates.io

@dvdplm
Copy link
Contributor

dvdplm commented Sep 5, 2019

@koushiro we will. Our policy is to test changes in parity-ethereum before releasing new versions. Once we have a PR there that passes all tests I'm fine making a release. Works for you?

@koushiro
Copy link
Contributor Author

koushiro commented Sep 5, 2019

@dvdplm Thanks for your response, it works for me :)

@niklasad1
Copy link
Member

@koushiro can you please fix the conflicts? Then we can merge it nice work 👍

@koushiro
Merge remote-tracking branch 'upstream/master' into rlp-no-std
286f137 
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro
Copy link
Contributor Author

koushiro commented Sep 5, 2019

@niklasad1 Done :)

@ordian ordian merged commit 63b6ee3 into paritytech:master Sep 5, 2019
@koushiro koushiro deleted the rlp-no-std branch September 5, 2019 15:55
ordian pushed a commit that referenced this pull request Sep 6, 2019
Merge branch 'master' into ao-kvdb-indexeddb
1e89da3 
* master:
  [triehash] Migrate to 2018 edition (#214)
  [rlp] Add no_std support  (#206)
@niklasad1 niklasad1 mentioned this pull request Oct 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@ordian ordian ordian approved these changes

Assignees
No one assigned
Labels
A8-looksgood
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[rlp] convert benches to criterion
4 participants
@koushiro @dvdplm @niklasad1 @ordian