Conversation
Trying to see if the logic around "lost" packets is correct. This test should fail as it misses packet 8
|
I fixed the bug and updated the code to simplify and try to explain more betterer whats going on. |
| if i > b.current && i < b.current+b.length { | ||
| // In between current and i need to be zero'd to allow those packets to come in later | ||
| for n := b.current + 1; n < i; n++ { | ||
| b.bits[n%b.length] = false |
There was a problem hiding this comment.
The original issue was not tracking lost for these zeroed out counter values
| if b.current == i { | ||
| // If i is within the current window but below the current counter, | ||
| // Check to see if it's a duplicate | ||
| if i > b.current-b.length || i < b.length && b.current < b.length { |
There was a problem hiding this comment.
this statement frightens the C programmer
There was a problem hiding this comment.
Can break it back out again
There was a problem hiding this comment.
eh I think it's find, the most I'd do is add some () around things to strongly communicate order
but golang already has sane evaluation order so we probably don't even need to do that
There was a problem hiding this comment.
I'm surprised go fmt doesn't add the parentheses for you, I appreciate that prettier adds them to make evaluation order explicitly obvious.
wadey
left a comment
wadey
left a comment
There was a problem hiding this comment.
Your fixes LGTM, need to find someone else to review though since I opened the PR with the failing test.
jrwren
left a comment
jrwren
left a comment
There was a problem hiding this comment.
am I crazy to not study it too thoroughly because it seems like there are good tests in place? LGTM
| if i > b.length && b.bits[i%b.length] == false { | ||
| // Check if the oldest bit was lost since we are shifting the window by 1 and occupying it with this counter | ||
| // The very first window can only be tracked as lost once we are on the 2nd window or greater | ||
| if b.bits[i%b.length] == false && i > b.length { |
There was a problem hiding this comment.
any particular reason the left and ride side of && are swapped from what they were before? I don't know how good the Go compiler is at optimizing this, but at first glance it seems like i > b.length would be the less costly operation and would be good to do first?
There was a problem hiding this comment.
Silly micro-op since i > b.length is more than likely always true. Our window size is 1024 currently, after which every packet would pass that part of the if statement.
5 participants
Trying to see if the logic around "lost" packets is correct. This test should show lost packets as it misses packet 8, but the test currently fails because we don't note the lost packet.