fix: LinearMessageView begin does not compare equal to begin (#969)

### Public-Facing Changes

bugfix: Multiple iterators created from LinearMessageView::begin() will compare to `true`.
### Description

<!-- describe what has changed, and motivation behind those changes -->

<!-- Link relevant Github issues. Use `Fixes #1234` to auto-close the
issue after merging. -->

.github/workflows/ci.yml

@@ -169,8 +169,6 @@ jobs:
         with:
           path: ~/.conan/data
           key: ${{ runner.os }}-${{ hashFiles('cpp/**/conanfile.py') }}
-      - uses: satackey/[email protected]
-        continue-on-error: true
       - run: make ci-format-check
       - run: make ci
       - run: make test-host

cpp/bench/conanfile.py

@@ -4,7 +4,7 @@
 class McapBenchmarksConan(ConanFile):
     settings = "os", "compiler", "build_type", "arch"
     generators = "cmake"
-    requires = "benchmark/1.7.0", "mcap/1.2.0"
+    requires = "benchmark/1.7.0", "mcap/1.2.1"
 
     def build(self):
         cmake = CMake(self)

cpp/build-docs.sh

@@ -4,7 +4,7 @@ set -e
 
 conan config init
 
-conan editable add ./mcap mcap/1.2.0
+conan editable add ./mcap mcap/1.2.1
 conan install docs --install-folder docs/build/Release \
   -s compiler.cppstd=17 -s build_type=Release --build missing
 

cpp/build.sh

@@ -4,7 +4,7 @@ set -e
 
 conan config init
 
-conan editable add ./mcap mcap/1.2.0
+conan editable add ./mcap mcap/1.2.1
 conan install test --install-folder test/build/Debug \
   -s compiler.cppstd=17 -s build_type=Debug --build missing
 

cpp/docs/conanfile.py

@@ -4,7 +4,7 @@
 class McapDocsConan(ConanFile):
     settings = "os", "compiler", "build_type", "arch"
     generators = "cmake"
-    requires = "mcap/1.2.0"
+    requires = "mcap/1.2.1"
 
     def build(self):
         cmake = CMake(self)

cpp/examples/conanfile.py

@@ -5,7 +5,7 @@ class McapExamplesConan(ConanFile):
     settings = "os", "compiler", "build_type", "arch"
     generators = "cmake"
     requires = [
-        "mcap/1.2.0",
+        "mcap/1.2.1",
         "protobuf/3.21.1",
         "nlohmann_json/3.10.5",
         "catch2/2.13.8",

cpp/mcap/conanfile.py

@@ -3,7 +3,7 @@
 
 class McapConan(ConanFile):
     name = "mcap"
-    version = "1.2.0"
+    version = "1.2.1"
     url = "https://github.com/foxglove/mcap"
     homepage = "https://github.com/foxglove/mcap"
     description = "A C++ implementation of the MCAP file format"

cpp/mcap/include/mcap/reader.hpp

@@ -657,13 +657,11 @@ struct MCAP_PUBLIC LinearMessageView {
     friend LinearMessageView;
 
     Iterator() = default;
-    Iterator(McapReader& mcapReader, ByteOffset dataStart, ByteOffset dataEnd,
-             const ReadMessageOptions& options, const ProblemCallback& onProblem);
+    Iterator(LinearMessageView& view);
 
     class Impl {
     public:
-      Impl(McapReader& mcapReader, ByteOffset dataStart, ByteOffset dataEnd,
-           const ReadMessageOptions& options, const ProblemCallback& onProblem);
+      Impl(LinearMessageView& view);
 
       Impl(const Impl&) = delete;
       Impl& operator=(const Impl&) = delete;
@@ -674,18 +672,18 @@ struct MCAP_PUBLIC LinearMessageView {
       reference dereference() const;
       bool has_value() const;
 
-      McapReader& mcapReader_;
+      LinearMessageView& view_;
+
       std::optional<TypedRecordReader> recordReader_;
       std::optional<IndexedMessageReader> indexedMessageReader_;
-      ReadMessageOptions readMessageOptions_;
-      const ProblemCallback& onProblem_;
       Message curMessage_;
       std::optional<MessageView> curMessageView_;
 
     private:
       void onMessage(const Message& message, RecordOffset offset);
     };
 
+    bool begun_ = false;
     std::unique_ptr<Impl> impl_;
   };
 

cpp/mcap/include/mcap/reader.inl

@@ -1588,8 +1588,7 @@ LinearMessageView::Iterator LinearMessageView::begin() {
   if (dataStart_ == dataEnd_ || !mcapReader_.dataSource()) {
     return end();
   }
-  return LinearMessageView::Iterator{mcapReader_, dataStart_, dataEnd_, readMessageOptions_,
-                                     onProblem_};
+  return LinearMessageView::Iterator{*this};
 }
 
 LinearMessageView::Iterator LinearMessageView::end() {
@@ -1598,37 +1597,27 @@ LinearMessageView::Iterator LinearMessageView::end() {
 
 // LinearMessageView::Iterator /////////////////////////////////////////////////
 
-LinearMessageView::Iterator::Iterator(McapReader& mcapReader, ByteOffset dataStart,
-                                      ByteOffset dataEnd,
-                                      const ReadMessageOptions& readMessageOptions,
-                                      const ProblemCallback& onProblem)
-    : impl_(std::make_unique<Impl>(mcapReader, dataStart, dataEnd, readMessageOptions, onProblem)) {
+LinearMessageView::Iterator::Iterator(LinearMessageView& view)
+    : impl_(std::make_unique<Impl>(view)) {
   if (!impl_->has_value()) {
     impl_ = nullptr;
   }
 }
 
-LinearMessageView::Iterator::Impl::Impl(McapReader& mcapReader, ByteOffset dataStart,
-                                        ByteOffset dataEnd,
-                                        const ReadMessageOptions& readMessageOptions,
-                                        const ProblemCallback& onProblem)
-    : mcapReader_(mcapReader)
-    , readMessageOptions_(readMessageOptions)
-    , onProblem_(onProblem) {
-  auto optionsStatus = readMessageOptions_.validate();
-  if (!optionsStatus.ok()) {
-    onProblem(optionsStatus);
-  }
-  if (readMessageOptions_.readOrder == ReadMessageOptions::ReadOrder::FileOrder) {
-    recordReader_.emplace(*mcapReader.dataSource(), dataStart, dataEnd);
+LinearMessageView::Iterator::Impl::Impl(LinearMessageView& view) : view_(view) {
+  auto dataStart = view.dataStart_;
+  auto dataEnd = view.dataEnd_;
+  auto readMessageOptions = view.readMessageOptions_;
+  if (readMessageOptions.readOrder == ReadMessageOptions::ReadOrder::FileOrder) {
+    recordReader_.emplace(*(view_.mcapReader_.dataSource()), dataStart, dataEnd);
 
     recordReader_->onSchema = [this](const SchemaPtr schema, ByteOffset,
                                      std::optional<ByteOffset>) {
-      mcapReader_.schemas_.insert_or_assign(schema->id, schema);
+      view_.mcapReader_.schemas_.insert_or_assign(schema->id, schema);
     };
     recordReader_->onChannel = [this](const ChannelPtr channel, ByteOffset,
                                       std::optional<ByteOffset>) {
-      mcapReader_.channels_.insert_or_assign(channel->id, channel);
+      view_.mcapReader_.channels_.insert_or_assign(channel->id, channel);
     };
     recordReader_->onMessage = [this](const Message& message, ByteOffset messageStartOffset,
                                       std::optional<ByteOffset> chunkStartOffset) {
@@ -1638,7 +1627,7 @@ LinearMessageView::Iterator::Impl::Impl(McapReader& mcapReader, ByteOffset dataS
       onMessage(message, offset);
     };
   } else {
-    indexedMessageReader_.emplace(mcapReader, readMessageOptions_,
+    indexedMessageReader_.emplace(view_.mcapReader_, readMessageOptions,
                                   std::bind(&LinearMessageView::Iterator::Impl::onMessage, this,
                                             std::placeholders::_1, std::placeholders::_2));
   }
@@ -1652,15 +1641,15 @@ LinearMessageView::Iterator::Impl::Impl(McapReader& mcapReader, ByteOffset dataS
  */
 void LinearMessageView::Iterator::Impl::onMessage(const Message& message, RecordOffset offset) {
   // make sure the message is within the expected time range
-  if (message.logTime < readMessageOptions_.startTime) {
+  if (message.logTime < view_.readMessageOptions_.startTime) {
     return;
   }
-  if (message.logTime >= readMessageOptions_.endTime) {
+  if (message.logTime >= view_.readMessageOptions_.endTime) {
     return;
   }
-  auto maybeChannel = mcapReader_.channel(message.channelId);
+  auto maybeChannel = view_.mcapReader_.channel(message.channelId);
   if (!maybeChannel) {
-    onProblem_(
+    view_.onProblem_(
       Status{StatusCode::InvalidChannelId,
              internal::StrCat("message at log_time ", message.logTime, " (seq ", message.sequence,
                               ") references missing channel id ", message.channelId)});
@@ -1669,14 +1658,14 @@ void LinearMessageView::Iterator::Impl::onMessage(const Message& message, Record
 
   auto& channel = *maybeChannel;
   // make sure the message is on the right topic
-  if (readMessageOptions_.topicFilter && !readMessageOptions_.topicFilter(channel.topic)) {
+  if (view_.readMessageOptions_.topicFilter && !view_.readMessageOptions_.topicFilter(channel.topic)) {
     return;
   }
   SchemaPtr maybeSchema;
   if (channel.schemaId != 0) {
-    maybeSchema = mcapReader_.schema(channel.schemaId);
+    maybeSchema = view_.mcapReader_.schema(channel.schemaId);
     if (!maybeSchema) {
-      onProblem_(Status{StatusCode::InvalidSchemaId,
+      view_.onProblem_(Status{StatusCode::InvalidSchemaId,
                         internal::StrCat("channel ", channel.id, " (", channel.topic,
                                          ") references missing schema id ", channel.schemaId)});
       return;
@@ -1698,7 +1687,7 @@ void LinearMessageView::Iterator::Impl::increment() {
       // Surface any problem that may have occurred while reading
       auto& status = recordReader_->status();
       if (!status.ok()) {
-        onProblem_(status);
+        view_.onProblem_(status);
       }
 
       if (!found) {
@@ -1714,7 +1703,7 @@ void LinearMessageView::Iterator::Impl::increment() {
         // alert with onProblem_.
         auto status = indexedMessageReader_->status();
         if (!status.ok()) {
-          onProblem_(status);
+          view_.onProblem_(status);
         }
         indexedMessageReader_ = std::nullopt;
         return;
@@ -1740,6 +1729,7 @@ LinearMessageView::Iterator::pointer LinearMessageView::Iterator::operator->() c
 }
 
 LinearMessageView::Iterator& LinearMessageView::Iterator::operator++() {
+  begun_ = true;
   impl_->increment();
   if (!impl_->has_value()) {
     impl_ = nullptr;
@@ -1752,7 +1742,17 @@ void LinearMessageView::Iterator::operator++(int) {
 }
 
 bool operator==(const LinearMessageView::Iterator& a, const LinearMessageView::Iterator& b) {
-  return a.impl_ == b.impl_;
+  if (a.impl_ == nullptr || b.impl_ == nullptr) {
+    // special case for Iterator::end() == Iterator::end()
+    return a.impl_ == b.impl_;
+  }
+  if (!a.begun_ && !b.begun_) {
+    // special case for Iterator::begin() == Iterator::begin()
+    // comparing iterators to the beginning of the same view should return true.
+    return &(a.impl_->view_) == &(b.impl_->view_);
+  }
+  // In all other cases, compare by object identity.
+  return &(a) == &(b);
 }
 
 bool operator!=(const LinearMessageView::Iterator& a, const LinearMessageView::Iterator& b) {

cpp/mcap/include/mcap/types.hpp

@@ -13,7 +13,7 @@
 
 namespace mcap {
 
-#define MCAP_LIBRARY_VERSION "1.2.0"
+#define MCAP_LIBRARY_VERSION "1.2.1"
 
 using SchemaId = uint16_t;
 using ChannelId = uint16_t;

cpp/test/conanfile.py

@@ -4,7 +4,7 @@
 class McapTestConan(ConanFile):
     settings = "os", "compiler", "build_type", "arch"
     generators = "cmake"
-    requires = "catch2/2.13.8", "mcap/1.2.0", "nlohmann_json/3.10.5"
+    requires = "catch2/2.13.8", "mcap/1.2.1", "nlohmann_json/3.10.5"
 
     def build(self):
         cmake = CMake(self)

cpp/test/unit_tests.cpp

@@ -476,6 +476,56 @@ TEST_CASE("McapReader::readMessages()", "[reader]") {
 
     reader.close();
   }
+
+  SECTION("IteratorComparison") {
+    Buffer buffer;
+
+    mcap::McapWriter writer;
+    writer.open(buffer, mcap::McapWriterOptions("test"));
+    mcap::Schema schema("schema", "schemaEncoding", "ab");
+    writer.addSchema(schema);
+    mcap::Channel channel("topic", "messageEncoding", schema.id);
+    writer.addChannel(channel);
+    std::vector<std::byte> data = {std::byte(1), std::byte(2), std::byte(3)};
+    WriteMsg(writer, channel.id, 0, 2, 1, data);
+    WriteMsg(writer, channel.id, 1, 4, 3, data);
+    writer.close();
+
+    mcap::McapReader reader;
+    requireOk(reader.open(buffer));
+
+    auto view = reader.readMessages();
+    auto it = view.begin();
+    REQUIRE(it != view.end());
+    REQUIRE(it == view.begin());
+    REQUIRE(it == it);
+    ++it;
+    REQUIRE(it != view.end());
+    REQUIRE(it != view.begin());
+    REQUIRE(it == it);
+    ++it;
+    REQUIRE(it == view.end());
+    REQUIRE(it != view.begin());
+    REQUIRE(it == it);
+
+    reader.close();
+  }
+  SECTION("IteratorComparisonEmpty") {
+    Buffer buffer;
+
+    mcap::McapWriter writer;
+    writer.open(buffer, mcap::McapWriterOptions("test"));
+    writer.close();
+
+    mcap::McapReader reader;
+    requireOk(reader.open(buffer));
+
+    auto view = reader.readMessages();
+    auto it = view.begin();
+    REQUIRE(it == view.begin());
+    REQUIRE(it == view.end());
+    reader.close();
+  }
 }
 
 /**