CL33-09: Make seq num range resilient to mixed up beginning and end. #404
Conversation
winder
requested review from
rstout,
asoliman92,
makramkd,
mateusz-sekara,
0xAustinWang and
b-gopalswami
|
| @@ -38,6 +38,9 @@ func (s SeqNum) String() string { | |||
| } | |||
|
|
|||
| func NewSeqNumRange(start, end SeqNum) SeqNumRange { | |||
| if end < start { | |||
There was a problem hiding this comment.
should we even allow this to begin with?
There was a problem hiding this comment.
I went back and forth about this question. This could generate an error but would require updating >150 spots where we use it.
| @@ -96,6 +99,9 @@ func (s SeqNumRange) String() string { | |||
| } | |||
|
|
|||
| func (s SeqNumRange) Length() int { | |||
| if s.End() < s.Start() { | |||
There was a problem hiding this comment.
This check is probably not needed considering you've already covered that on the constructor level
There was a problem hiding this comment.
Also, it's misleading to shuffle with the state in the read method. If you need a defensive check then probably calling absolute value on the computed length should work, no?
| @@ -38,6 +38,9 @@ func (s SeqNum) String() string { | |||
| } | |||
|
|
|||
| func NewSeqNumRange(start, end SeqNum) SeqNumRange { | |||
| if end < start { | |||
There was a problem hiding this comment.
Hmm, I'm not sure about that. Shouldn't we error when someone mixes start with the end? We could hide some underlying bug when "healing" that state during runtime
Avoid overflow when End < Start by swapping values to be in the correct order.