Refactor validator windowing #882
Conversation
pgarg66
commented
Aug 6, 2018
pgarg66
commented
- a unit test for windowing functions
- issue refactor validator windowing and write a unit test for windowing functions #857
pgarg66
force-pushed
the
refactor-validator-windowing
branch
from
a9ecf9d to
7d65fc0
Compare
src/streamer.rs
Outdated
| pix: u64, | ||
| consumed: u64, | ||
| received: &mut u64, | ||
| ) -> bool { |
There was a problem hiding this comment.
with u64 carrying received?
There was a problem hiding this comment.
@garious , wondering how Option helps here. Is this to differentiate between different error conditions? Or something more?
There was a problem hiding this comment.
Yes, with u64 carrying received. It's more idiomatic Rust to avoid mut. You'll see those tests get shorter.
pgarg66
force-pushed
the
refactor-validator-windowing
branch
from
7d65fc0 to
86ccd84
Compare
|
@garious , please see the latest patch. It simplified the tests, as you had mentioned. |
src/streamer.rs
Outdated
| return None; | ||
| } | ||
|
|
||
| if pix > received { |
There was a problem hiding this comment.
Some(cmp::max(pix, received))
src/streamer.rs
Outdated
| ); | ||
|
|
||
| let result = validate_blob_against_window(debug_id, pix, *consumed, *received); | ||
| if result.is_none() { |
There was a problem hiding this comment.
Can you use a match here? Under the hood, both is_none() and result.unwrap() below would use a match. I'm guessing clippy is about to suggest this as well. Let's see!
There was a problem hiding this comment.
Funny, as I used match initially, and then changed it to this. I'll change it to match.
src/streamer.rs
Outdated
| #[test] | ||
| pub fn validate_blob_against_window_test() { | ||
| let recvd: u64 = 100; | ||
| assert!(validate_blob_against_window(0, 90 + WINDOW_SIZE, 90, recvd).is_none()); |
There was a problem hiding this comment.
Can you use assert_eq! in all of these? It'll give you better error messages.
There was a problem hiding this comment.
Sure. I'll change it.
src/streamer.rs
Outdated
| ) -> Option<u64> { | ||
| // Prevent receive window from running over | ||
| if pix >= consumed + WINDOW_SIZE { | ||
| debug!( |
There was a problem hiding this comment.
Does it bother anyone that this information is lost if not logging in debug mode? I see the original code did this, so I'm not going to ask you to clean that up here, but could you add a ticket to add an error type and change this function from Option<u64> to Result<u64>?
- a unit test for windowing functions - issue anza-xyz#857
pgarg66
force-pushed
the
refactor-validator-windowing
branch
from
86ccd84 to
35ce82a
Compare
|
@garious , this should address most of the comments. |
* some test tweaks * get rid of some redundancy
