Merge pull request #3153 from pleroy/IteratorFixes

Fix bugs found while writing tests for DiscreteTrajectory2

Author: pleroy

physics/discrete_trajectory_iterator.hpp

@@ -68,10 +68,6 @@ class DiscreteTrajectoryIterator {
   DiscreteTrajectorySegmentIterator<Frame> segment_;
   OptionalTimelineConstIterator point_;
 
-  // The last time that was seen by the iterator.  Used to skip over repeated
-  // times.
-  std::optional<Instant> previous_time_;
-
   template<typename F>
   friend class physics::DiscreteTrajectorySegment;
   friend class physics::DiscreteTrajectoryIteratorTest;

physics/discrete_trajectory_iterator_body.hpp

@@ -15,10 +15,11 @@ DiscreteTrajectoryIterator<Frame>&
 DiscreteTrajectoryIterator<Frame>::operator++() {
   CHECK(!is_at_end(point_));
   auto& point = iterator(point_);
-  for (;;) {
+  Instant const previous_time = point->first;
+  do {
     if (point == --segment_->timeline_end()) {
       ++segment_;
-      if (segment_ == segment_.end()) {
+      if (segment_.is_end() || segment_->timeline_empty()) {
         point_.reset();
         break;
       } else {
@@ -27,36 +28,32 @@ DiscreteTrajectoryIterator<Frame>::operator++() {
     } else {
       ++point;
     }
-    if (point->first != previous_time_) {
-      previous_time_ = point->first;
-      break;
-    }
-  }
+  } while (point->first == previous_time);
   return *this;
 }
 
 template<typename Frame>
 DiscreteTrajectoryIterator<Frame>&
 DiscreteTrajectoryIterator<Frame>::operator--() {
-  if (is_at_end(point_)) {
-    segment_ = --segment_.end();
-    point_ = --segment_->timeline_end();
-  } else {
-    auto& point = iterator(point_);
-    for (;;) {
-      if (point == segment_->timeline_begin()) {
-        CHECK(segment_ != segment_.begin());
-        --segment_;
-        point = --segment_->timeline_end();
-      } else {
-        --point;
-      }
-      if (point->first != previous_time_) {
-        previous_time_ = point->first;
-        break;
-      }
-    }
+  bool const point_is_at_end = is_at_end(point_);
+  if (point_is_at_end) {
+    // Move the iterator to the end of the last segment.
+    segment_ = --segment_.segments().end();
+    point_ = segment_->timeline_end();
+    // Now proceed with the decrement.
   }
+  auto& point = iterator(point_);
+  std::optional<Instant> const previous_time =
+      point_is_at_end ? std::nullopt : std::make_optional(point->first);
+  do {
+    if (point == segment_->timeline_begin()) {
+      CHECK(!segment_.is_begin());
+      --segment_;
+      point = --segment_->timeline_end();
+    } else {
+      --point;
+    }
+  } while (point->first == previous_time);
   return *this;
 }
 
@@ -113,7 +110,11 @@ DiscreteTrajectoryIterator<Frame>::DiscreteTrajectoryIterator(
     DiscreteTrajectorySegmentIterator<Frame> const segment,
     OptionalTimelineConstIterator const point)
     : segment_(segment),
-      point_(point) {}
+      point_(point) {
+  if (segment_.is_end() || segment_->timeline_empty()) {
+    point_.reset();
+  }
+}
 
 template<typename Frame>
 bool DiscreteTrajectoryIterator<Frame>::is_at_end(

physics/discrete_trajectory_iterator_test.cpp

@@ -61,17 +61,20 @@ class DiscreteTrajectoryIteratorTest : public ::testing::Test {
     }
   }
 
+  void Append(Segments::iterator const it,
+              Instant const& t,
+              DegreesOfFreedom<World> const& degrees_of_freedom) {
+    it->Append(t, degrees_of_freedom);
+  }
+
   DiscreteTrajectoryIterator<World> MakeBegin(
       Segments::const_iterator const it) {
-    return DiscreteTrajectoryIterator<World>(
-        DiscreteTrajectorySegmentIterator<World>(segments_.get(), it),
-        it->timeline_begin());
+    return it->begin();
   }
 
-  DiscreteTrajectoryIterator<World> MakeEnd(Segments::const_iterator it) {
-    return DiscreteTrajectoryIterator<World>(
-        DiscreteTrajectorySegmentIterator<World>(segments_.get(), ++it),
-        std::nullopt);
+  DiscreteTrajectoryIterator<World> MakeEnd(
+      Segments::const_iterator const it) {
+    return it->end();
   }
 
   // Constructs a list of |n| segments which are properly initialized.
@@ -167,5 +170,47 @@ TEST_F(DiscreteTrajectoryIteratorTest, Equality) {
   EXPECT_NE(MakeBegin(segments_->begin()), MakeEnd(--segments_->end()));
 }
 
+// Empty segments may exist in a transient manner, we must be able to iterate
+// over them.
+TEST_F(DiscreteTrajectoryIteratorTest, EmptySegment) {
+  auto segments = MakeSegments(1);
+  {
+    int count = 0;
+    for (auto const& point : segments->front()) {
+      ++count;
+    }
+    EXPECT_EQ(0, count);
+  }
+  {
+    int count = 0;
+    for (auto it = segments->front().rbegin();
+         it != segments->front().rend();
+         ++it) {
+      ++count;
+    }
+    EXPECT_EQ(0, count);
+  }
+}
+
+// Check that repeated points don't cause confusion regarding the end of a
+// segment.
+TEST_F(DiscreteTrajectoryIteratorTest, SegmentEnd) {
+  auto segment0 = segments_->begin();
+  auto segment1 = std::next(segment0);
+  auto iterator = segment0->begin();
+  for (int i = 0; i < 5; ++i) {
+    ++iterator;
+  }
+  EXPECT_TRUE(iterator != segment1->end());
+}
+
+// Checkt that rbegin() works if the next segment is empty.
+TEST_F(DiscreteTrajectoryIteratorTest, EmptyLastSegment) {
+  auto segments = MakeSegments(2);
+  auto segment = segments->begin();
+  Append(segment, t0_, unmoving_origin_);
+  EXPECT_EQ(t0_, segment->rbegin()->first);
+}
+
 }  // namespace physics
 }  // namespace principia

physics/discrete_trajectory_segment.hpp

@@ -52,7 +52,7 @@ class DiscreteTrajectorySegment : public Trajectory<Frame> {
   explicit DiscreteTrajectorySegment(
       DiscreteTrajectorySegmentIterator<Frame> self);
 
-  virtual ~DiscreteTrajectorySegment() = default;
+  ~DiscreteTrajectorySegment() = default;
 
   // Moveable.
   DiscreteTrajectorySegment(DiscreteTrajectorySegment&&) = default;
@@ -69,7 +69,7 @@ class DiscreteTrajectorySegment : public Trajectory<Frame> {
 
   // TODO(phl): We probably don't want empty segments.
   bool empty() const;
-  virtual std::int64_t size() const;
+  std::int64_t size() const;
 
   iterator find(Instant const& t) const;
 
@@ -120,8 +120,9 @@ class DiscreteTrajectorySegment : public Trajectory<Frame> {
   Hermite3<Instant, Position<Frame>> GetInterpolation(
       typename Timeline::const_iterator upper) const;
 
-  virtual typename Timeline::const_iterator timeline_begin() const;
-  virtual typename Timeline::const_iterator timeline_end() const;
+  typename Timeline::const_iterator timeline_begin() const;
+  typename Timeline::const_iterator timeline_end() const;
+  bool timeline_empty() const;
 
   std::optional<DownsamplingParameters> downsampling_parameters_;
 

physics/discrete_trajectory_segment_body.hpp

@@ -305,6 +305,11 @@ DiscreteTrajectorySegment<Frame>::timeline_end() const {
   return timeline_.cend();
 }
 
+template<typename Frame>
+bool DiscreteTrajectorySegment<Frame>::timeline_empty() const {
+  return timeline_.empty();
+}
+
 }  // namespace internal_discrete_trajectory_segment
 }  // namespace physics
 }  // namespace principia

physics/discrete_trajectory_segment_iterator.hpp

@@ -3,6 +3,7 @@
 #include "absl/container/btree_map.h"
 #include "base/macros.hpp"
 #include "base/not_null.hpp"
+#include "physics/discrete_trajectory_segment_range.hpp"
 #include "physics/discrete_trajectory_types.hpp"
 
 namespace principia {
@@ -57,8 +58,10 @@ class DiscreteTrajectorySegmentIterator {
   DiscreteTrajectorySegmentIterator(not_null<Segments const*> segments,
                                     typename Segments::const_iterator iterator);
 
-  DiscreteTrajectorySegmentIterator begin() const;
-  DiscreteTrajectorySegmentIterator end() const;
+  bool is_begin() const;
+  bool is_end() const;
+  DiscreteTrajectorySegmentRange<DiscreteTrajectorySegmentIterator>
+  segments() const;
 
   // Not not_null<> to be default-constructible.
   Segments const* segments_ = nullptr;

physics/discrete_trajectory_segment_iterator_body.hpp

@@ -62,15 +62,20 @@ DiscreteTrajectorySegmentIterator<Frame>::DiscreteTrajectorySegmentIterator(
       iterator_(iterator) {}
 
 template<typename Frame>
-DiscreteTrajectorySegmentIterator<Frame>
-DiscreteTrajectorySegmentIterator<Frame>::begin() const {
-  return DiscreteTrajectorySegmentIterator(segments_, segments_->begin());
+bool DiscreteTrajectorySegmentIterator<Frame>::is_begin() const {
+  return iterator_ == segments_->begin();
 }
 
 template<typename Frame>
-DiscreteTrajectorySegmentIterator<Frame>
-DiscreteTrajectorySegmentIterator<Frame>::end() const {
-  return DiscreteTrajectorySegmentIterator(segments_, segments_->end());
+bool DiscreteTrajectorySegmentIterator<Frame>::is_end() const {
+  return iterator_ == segments_->end();
+}
+
+template<typename Frame>
+DiscreteTrajectorySegmentRange<DiscreteTrajectorySegmentIterator<Frame>>
+DiscreteTrajectorySegmentIterator<Frame>::segments() const {
+  return {DiscreteTrajectorySegmentIterator(segments_, segments_->begin()),
+          DiscreteTrajectorySegmentIterator(segments_, segments_->end())};
 }
 
 }  // namespace internal_discrete_trajectory_segment_iterator