dcsctp: Make Sequence Number API more consistent
* `AddTo` and `Difference` are made into static methods, as one may have
believed that these modified the current object previously. The
`Increment` method is kept, as it's obvious that it modifies the
current object as it doesn't have a return value, and `next_value` is
kept, as its naming (lower-case, snake) indicates that it's a simple
accessor.
* Difference will return the absolute difference. This is actually the
only reasonable choice, as the return value was unsigned and any
negative value would just wrap.
Bug: webrtc:12614
Change-Id: If14a71636e67fc612d12759dc80a9c2518c85281
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215069
Reviewed-by: Tommi <tommi@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33714}
diff --git a/net/dcsctp/common/sequence_numbers.h b/net/dcsctp/common/sequence_numbers.h
index 4f318c3..52b638b 100644
--- a/net/dcsctp/common/sequence_numbers.h
+++ b/net/dcsctp/common/sequence_numbers.h
@@ -121,19 +121,26 @@
// Increments the value.
void Increment() { ++value_; }
+
+ // Returns the next value relative to this sequence number.
UnwrappedSequenceNumber<WrappedType> next_value() const {
return UnwrappedSequenceNumber<WrappedType>(value_ + 1);
}
- // Adds a delta to the current value.
- UnwrappedSequenceNumber<WrappedType> AddTo(int delta) const {
- return UnwrappedSequenceNumber<WrappedType>(value_ + delta);
+ // Returns a new sequence number based on `value`, and adding `delta` (which
+ // may be negative).
+ static UnwrappedSequenceNumber<WrappedType> AddTo(
+ UnwrappedSequenceNumber<WrappedType> value,
+ int delta) {
+ return UnwrappedSequenceNumber<WrappedType>(value.value_ + delta);
}
- // Compares the difference between two sequence numbers.
- typename WrappedType::UnderlyingType Difference(
- UnwrappedSequenceNumber<WrappedType> other) const {
- return value_ - other.value_;
+ // Returns the absolute difference between `lhs` and `rhs`.
+ static typename WrappedType::UnderlyingType Difference(
+ UnwrappedSequenceNumber<WrappedType> lhs,
+ UnwrappedSequenceNumber<WrappedType> rhs) {
+ return (lhs.value_ > rhs.value_) ? (lhs.value_ - rhs.value_)
+ : (rhs.value_ - lhs.value_);
}
private:
diff --git a/net/dcsctp/common/sequence_numbers_test.cc b/net/dcsctp/common/sequence_numbers_test.cc
index d1d1959..f5fa788 100644
--- a/net/dcsctp/common/sequence_numbers_test.cc
+++ b/net/dcsctp/common/sequence_numbers_test.cc
@@ -32,9 +32,9 @@
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
- EXPECT_EQ(s1.Difference(s0), 1);
- EXPECT_EQ(s2.Difference(s0), 2);
- EXPECT_EQ(s3.Difference(s0), 3);
+ EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
+ EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
+ EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@@ -50,7 +50,7 @@
s2.Increment();
EXPECT_EQ(s2, s3);
- EXPECT_EQ(s0.AddTo(2), s3);
+ EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, MidValueUnwrapping) {
@@ -68,9 +68,9 @@
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
- EXPECT_EQ(s1.Difference(s0), 1);
- EXPECT_EQ(s2.Difference(s0), 2);
- EXPECT_EQ(s3.Difference(s0), 3);
+ EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
+ EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
+ EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@@ -86,7 +86,7 @@
s2.Increment();
EXPECT_EQ(s2, s3);
- EXPECT_EQ(s0.AddTo(2), s3);
+ EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, WrappedUnwrapping) {
@@ -104,9 +104,9 @@
EXPECT_LT(s1, s3);
EXPECT_LT(s2, s3);
- EXPECT_EQ(s1.Difference(s0), 1);
- EXPECT_EQ(s2.Difference(s0), 2);
- EXPECT_EQ(s3.Difference(s0), 3);
+ EXPECT_EQ(TestSequence::Difference(s1, s0), 1);
+ EXPECT_EQ(TestSequence::Difference(s2, s0), 2);
+ EXPECT_EQ(TestSequence::Difference(s3, s0), 3);
EXPECT_GT(s1, s0);
EXPECT_GT(s2, s0);
@@ -122,7 +122,7 @@
s2.Increment();
EXPECT_EQ(s2, s3);
- EXPECT_EQ(s0.AddTo(2), s3);
+ EXPECT_EQ(TestSequence::AddTo(s0, 2), s3);
}
TEST(SequenceNumbersTest, WrapAroundAFewTimes) {
@@ -183,5 +183,20 @@
}
}
+TEST(SequenceNumbersTest, DifferenceIsAbsolute) {
+ TestSequence::Unwrapper unwrapper;
+
+ TestSequence this_value = unwrapper.Unwrap(Wrapped(10));
+ TestSequence other_value = TestSequence::AddTo(this_value, 100);
+
+ EXPECT_EQ(TestSequence::Difference(this_value, other_value), 100);
+ EXPECT_EQ(TestSequence::Difference(other_value, this_value), 100);
+
+ TestSequence minus_value = TestSequence::AddTo(this_value, -100);
+
+ EXPECT_EQ(TestSequence::Difference(this_value, minus_value), 100);
+ EXPECT_EQ(TestSequence::Difference(minus_value, this_value), 100);
+}
+
} // namespace
} // namespace dcsctp
diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc
index 9e5cbe2..b95cb44 100644
--- a/net/dcsctp/rx/data_tracker.cc
+++ b/net/dcsctp/rx/data_tracker.cc
@@ -179,9 +179,10 @@
auto flush = [&]() {
if (first_tsn_in_block.has_value()) {
- int start_diff =
- first_tsn_in_block->Difference(last_cumulative_acked_tsn_);
- int end_diff = last_tsn_in_block->Difference(last_cumulative_acked_tsn_);
+ int start_diff = UnwrappedTSN::Difference(*first_tsn_in_block,
+ last_cumulative_acked_tsn_);
+ int end_diff = UnwrappedTSN::Difference(*last_tsn_in_block,
+ last_cumulative_acked_tsn_);
gap_ack_blocks.emplace_back(static_cast<uint16_t>(start_diff),
static_cast<uint16_t>(end_diff));
first_tsn_in_block = absl::nullopt;
diff --git a/net/dcsctp/rx/traditional_reassembly_streams.cc b/net/dcsctp/rx/traditional_reassembly_streams.cc
index caa97d2..7cec115 100644
--- a/net/dcsctp/rx/traditional_reassembly_streams.cc
+++ b/net/dcsctp/rx/traditional_reassembly_streams.cc
@@ -172,7 +172,8 @@
return 0;
}
- uint32_t tsn_diff = chunks.rbegin()->first.Difference(chunks.begin()->first);
+ uint32_t tsn_diff =
+ UnwrappedTSN::Difference(chunks.rbegin()->first, chunks.begin()->first);
if (tsn_diff != chunks.size() - 1) {
return 0;
}
