[rlp] Add no_std support by koushiro · Pull Request #167 · paritytech/parity-common

koushiro · 2019-05-27T06:35:01Z

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

What have I changed?

Add no_std support for rlp crate
Migrate rlp crate to 2018 edition
Remove some test cases that depend on primitive-types (These test cases should be added to primitive-types instead of rlp)

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

dvdplm

In general it looks good – we definitely want to move to the 2018 edition and no_std capability is always welcome. I don't understand why tests&benchmarks need to be no_std though, can you elaborate on that?

dvdplm · 2019-05-27T13:28:48Z

-rustc-hex = {version = "2.0", default-features = false }
-
-[dev-dependencies]
-hex-literal = "0.1"


Why prefer rustc-hex to hex-literal?

I think rustc-hex is enough to encode/decode hex. There is no need to introduce more dependencies, even if it is just dev-dependencies.

Tests need to be as readable as possible, which is why I thinkhex-literal is better.

(The new 0.2 of hex-literal is edition compatible too!)

dvdplm · 2019-05-27T13:34:17Z

 	let mut stream = RlpStream::new_list(1000);
 	for _ in 0..1000 {
-		stream.append(&U256::from(1));
+		stream.append(&1u64);


This changes the amount of data tested, making it impossible to compare tests before/after this change.

In general I'm not sure why the dev dependencies can't depend on primitive-types (or any other code, be it no_std or not). I think the benchmark use the big uint types because it is such a common use case for the crate. What's the point of having the benchmarks be no_std?

Please use the same data length as before here.

koushiro · 2019-05-28T03:36:17Z

@dvdplm Thanks for reviewing my code!

I removed the primitive-types dependency because H160 and U256 are custom types in primitive-types crate. If we want to test their rlp codec, I think we should write tests in primitive-types instead of in rlp crate.

For example, if you define a structure that implements a Serialize/Deserialize trait and you want to know if the serialization and deserialization of the structure is as expected, you should write tests in the crate of the structure you defined, instead of submitting a PR to request the serde-rs maintainers to add a dev-dependencies to test your code.

koushiro · 2019-05-28T03:49:18Z

I just make tests are no_std compatible, which is a good indication that the code is no problem in the no_std environment. As for the benchmarks of U256 type, I think it should also be written in primitive-types crate for the same reason above.

sorpaas

@koushiro Would you mind to move the removed tests to primitve-types crate? This PR looks good to me but in general I think it would be a good idea not to remove working tests.

dvdplm · 2019-05-28T07:27:49Z

@koushiro no, thank you for contributing! :)

So the context for my hesitation about moving the tests is that we've seen several instances of good PRs come in that look perfectly justified and that seem like an easy win and so they are merged but then when someone actually tries to use the new versions in production code, issues pop up. Most of the time it's ergonomic quirks but sometimes in the past we've merged code that turned out to be impossible to use higher up in the stack. I don't actually know what the best way to tackle this is – we can hardly expect all contributor to go through the hassle of upgrading depending crates/projects. The tests you remove here – and I totally understand your reasoning – makes me wonder if we're not removing one barrier to such unfortunate merges: the tests exercise a portion of the stack above itself so we know that part is sort of ok.

The obvious objection is that these are unit tests, not integration tests. And of course you and @sorpaas are right: these tests logically belong in primitive-types.

Maybe we can meet half-way:

revert the test removals here and keep only the no_std parts
in a separate PR, move the unit tests to where they belong and at the same time add a doc-test here that proves that rlp + primitive-types still work

Would that work for you?

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

koushiro · 2019-05-28T07:53:56Z

@dvdplm @sorpaas I will add more tests (include removed tests in this PR) into primitive-types in another PR. PTAL again.

dvdplm · 2019-05-28T09:26:51Z

 	let mut stream = RlpStream::new_list(1000);
 	for _ in 0..1000 {
-		stream.append(&U256::from(1));
+		stream.append(&1u64);


Please use the same data length as before here.

dvdplm · 2019-05-28T09:27:11Z

 		let rlp = Rlp::new(&data);
 		for i in 0..1000 {
-			let _: U256 = rlp.val_at(i).unwrap();
+			let _: u64 = rlp.val_at(i).unwrap();


Please use the same data length as before here.

dvdplm · 2019-05-28T09:27:46Z

 use byteorder::{ByteOrder, BigEndian};
-use crate::traits::{Encodable, Decodable};
+
+use crate::error::DecoderError;


Consider using

use crate::{ … … }

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

dvdplm · 2019-06-26T06:48:04Z

I will add more tests (include removed tests in this PR) into primitive-types in another PR

Let us know when this other PR is ready so we can unblock this one. :)

dvdplm · 2019-08-12T14:08:52Z

@koushiro I'll close this for now. Feel free to re-open at any time with the last changes requested.

koushiro added 4 commits May 27, 2019 14:22

Support no_std for rlp; Migrate rlp crate to 2018 edition

ecea884

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

Modify CI config to support the no_std test of rlp

bfca031

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

Remove useless test

17a5a21

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

Fix ci

bb76515

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

koushiro changed the title ~~Support no_std for rlp crate~~ [rlp] Add no_std support May 27, 2019

dvdplm suggested changes May 27, 2019

View reviewed changes

sorpaas reviewed May 28, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into rlp_no_std

89a6be2

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

dvdplm suggested changes May 28, 2019

View reviewed changes

Move tests

a137df0

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

koushiro mentioned this pull request May 30, 2019

Add no_std support for fixed-hash with feature rustc-hex #173

Merged

koushiro added 2 commits May 31, 2019 16:47

Merge remote-tracking branch 'upstream/master' into rlp_no_std

1ce5187

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

Merge remote-tracking branch 'upstream/master' into rlp_no_std

822933d

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

dvdplm closed this Aug 12, 2019

koushiro mentioned this pull request Aug 29, 2019

[rlp] Add no_std support #206

Merged

Conversation

koushiro commented May 27, 2019

What have I changed?

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

koushiro commented May 28, 2019

Uh oh!

koushiro commented May 28, 2019

Uh oh!

sorpaas left a comment

Choose a reason for hiding this comment

Uh oh!

dvdplm commented May 28, 2019

Uh oh!

koushiro commented May 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dvdplm commented Jun 26, 2019

Uh oh!

dvdplm commented Aug 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants