feat(core): Adopt two-phase construction for clp::ReadOnlyMemoryMappedFile; Remove boost::iostreams dependency in clp-s.

[Bill-hbrhbr]

Description

This PR follows #445 and #450.

The clp-ffi-js project will need to support clp-s code in the near future. Removing boost::iostreams from clp-s will help simplify the dependency resolution on the ffi side.

However, clp::ReadOnlyMemoryMappedFile used clp::TraceableException and clp::ErrorCode instead of the clp-s versions, resulting in unnecessary complexities when implementing error handling. Hence, we switch to the preferred rust-style error handling with ystdlib::error_handling::Result for clp::ReadOnlyMemoryMappedFile.

Checklist

• The PR satisfies the contribution guidelines.
• This is a breaking change and that has been indicated in the PR title, OR this isn't a
  breaking change.
• Necessary docs have been updated, OR no docs need to be updated.

Validation performed

• Existing unit tests pass.

Summary by CodeRabbit

• Refactor

  • Safer, errno-based error handling and clearer failure logging for memory-mapped file operations.
  • Reduced dependency on Boost.iostreams.

• API

  • Simplified construction pattern for memory-mapped files (public creation pattern updated; move-only semantics).

• Tests

  • Updated tests to exercise the new creation and error-checking flow for memory-mapped files.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Replaces ReadOnlyMemoryMappedFile's throwing constructor with a static create(std::string_view) factory that returns ystdlib::error_handling::Result; updates callers to handle result-based mapping, switches internal ownership to optional, and removes Boost::iostreams linkage where mapping wrapper is used.

Changes

Cohort / File(s)	Summary
ReadOnlyMemoryMappedFile implementation components/core/src/clp/ReadOnlyMemoryMappedFile.hpp, components/core/src/clp/ReadOnlyMemoryMappedFile.cpp	Replaces public path constructor with static create(std::string_view) returning ystdlib::error_handling::Result<ReadOnlyMemoryMappedFile>. Adds private ctor, m_data/m_buf_size members, deletes copy semantics, adds move semantics, changes destructor/munmap handling, and switches to errno-based error reporting.
Streaming archive reader components/core/src/clp/streaming_archive/reader/Segment.cpp	Replaces exception-based mapping with ReadOnlyMemoryMappedFile::create(...) result handling; logs errno-based errors and early-returns Failure on mapping errors.
Zstd decompressor (core) components/core/src/clp/streaming_compression/zstd/Decompressor.hpp, components/core/src/clp/streaming_compression/zstd/Decompressor.cpp	Uses factory create(...) for mapping and stores mapping in optional; updates setup to use result.value().get_view() and returns ErrorCode_Failure on mapping errors.
Zstd decompressor (clp_s) components/core/src/clp_s/ZstdDecompressor.hpp, components/core/src/clp_s/ZstdDecompressor.cpp	Replaces boost::iostreams::mapped_file_source with std::optional<clp::ReadOnlyMemoryMappedFile>; open()/close() updated to use create()/reset() and to configure input from get_view().
Build files components/core/src/clp_s/CMakeLists.txt, components/core/src/clp_s/indexer/CMakeLists.txt	Removes Boost::iostreams from target link libraries; adds ReadOnlyMemoryMappedFile source files to indexer sources.
Tests components/core/tests/test-MemoryMappedFile.cpp, components/core/tests/test-StreamingCompression.cpp	Switches tests to call ReadOnlyMemoryMappedFile::create(...), check has_error() and move value() into test-local mapping objects; preserves existing assertions on view contents.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: adopting two-phase construction for ReadOnlyMemoryMappedFile and removing boost::iostreams from clp-s, which align directly with the changeset modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

components/core/src/clp_s/indexer/CMakeLists.txt (1)

115-116: ⚠️ Potential issue | 🔴 Critical

Missing compilation units: ReadOnlyMemoryMappedFile.cpp and FileDescriptor.cpp not in indexer build.

The indexer compiles ../ZstdDecompressor.cpp (line 115) and includes ../../clp/streaming_compression/zstd/Decompressor.cpp (line 69), both of which depend on clp::ReadOnlyMemoryMappedFile. Since ReadOnlyMemoryMappedFile.cpp contains real implementations (mmap/munmap calls) and depends on FileDescriptor.cpp, both source files must be compiled for the indexer target.

Currently, neither ../../clp/ReadOnlyMemoryMappedFile.cpp nor ../../clp/FileDescriptor.cpp are in INDEXER_SOURCES, and the indexer does not link clp_s::clp_dependencies (which contains these files). This will cause linker errors.

Add both source files to INDEXER_SOURCES or link the indexer against clp_s::clp_dependencies.

components/core/src/clp_s/ZstdDecompressor.cpp (1)

244-271: ⚠️ Potential issue | 🟠 Major

Errno-based error handling on mmap failure looks correct.

The new open(std::string const&) properly:

• Constructs the ReadOnlyMemoryMappedFile via make_unique
• Checks is_open() using the false == pattern per coding guidelines
• Reports the errno with strerror on failure
• Configures the input stream from get_view()

One concern: on line 248, m_input_type is set to MemoryMappedCompressedFile before the open check on line 252. If is_open() returns false, the method returns ErrorCodeFailure with m_input_type still set to MemoryMappedCompressedFile, leaving the object in a partially-initialized state. A subsequent call to any other open() overload would throw ErrorCodeNotReady instead of succeeding. The caller must explicitly call close() to recover. Consider resetting m_input_type back to NotInitialized on the error path.

Proposed fix

     m_memory_mapped_file = std::make_unique<clp::ReadOnlyMemoryMappedFile>(compressed_file_path);
     if (false == m_memory_mapped_file->is_open()) {
         auto const error_number{m_memory_mapped_file->get_errno()};
         SPDLOG_ERROR(
                 "ZstdDecompressor: Unable to memory map the compressed file with path: {}. errno = "
                 "{} ({}).",
                 compressed_file_path.c_str(),
                 error_number,
                 std::strerror(error_number)
         );
+        m_memory_mapped_file.reset();
+        m_input_type = InputType::NotInitialized;
         return ErrorCodeFailure;
     }

🤖 Fix all issues with AI agents

In `@components/core/src/clp_s/ZstdDecompressor.cpp`:
- Line 8: Remove the stale Boost header include
"<boost/iostreams/device/mapped_file.hpp>" from ZstdDecompressor.cpp; this
include is no longer used because memory-mapping is handled via
clp::ReadOnlyMemoryMappedFile, so delete the include line and ensure any
lingering references to boost::iostreams::mapped_file are removed or replaced
with clp::ReadOnlyMemoryMappedFile usage (check code in ZstdDecompressor class
and related functions to confirm compilation).

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.cpp`:
- Around line 34-38: The log call in ReadOnlyMemoryMappedFile uses path.data()
(a std::string_view) which may not be null-terminated; change the SPDLOG_ERROR
invocation to pass the std::string_view itself (i.e., path) or explicitly
construct a std::string (e.g., std::string(path)) so the formatting treats it
safely, keeping the same message and ex.what() usage.
- Around line 14-40: The constructor currently returns early for empty files
leaving m_data==nullptr and m_errno==0 which makes is_open() report failure;
change the ReadOnlyMemoryMappedFile constructor to treat an empty file as a
successful open by explicitly marking the object open: set m_buf_size = 0,
m_data = nullptr (as now) and set a success flag (e.g. m_is_open = true) or
adjust is_open() to check an explicit "opened successfully" state instead of
m_data; ensure get_errno() remains 0 for this case and update any code paths
that rely on m_data to handle zero-length buffers (references:
ReadOnlyMemoryMappedFile::ReadOnlyMemoryMappedFile, is_open(), m_errno, m_data,
m_buf_size, get_errno()).

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.hpp`:
- Around line 46-51: The current is_open() implementation (checking m_data,
m_buf_size, and m_errno) treats a successfully opened zero-byte file as not
open; decide and make semantics explicit: either update the doc comment above
is_open() to state "Returns false for empty files since mmap cannot map
zero-length regions" so callers (which reference m_buf_size) don't log
strerror(0) on empty files, or change is_open() to treat
zero-length-but-successful opens as open by using: return m_errno == 0 &&
(m_data != nullptr || m_buf_size == 0); also update callers that log errors to
check m_buf_size==0 and avoid emitting strerror(0) for empty-file cases. Ensure
references to is_open(), m_data, m_buf_size, and m_errno are adjusted
consistently.

[LinZhihao-723]

Since we're here, I'd strongly prefer to rewrite this API with result-style error handling for better maintainability. What you will need to do:

• Enable move semantics:
  • Add a move constructor which swap the underlying pointer with nullptr.
  • In the destructor, if the data is nullptr then directly return.
• Make the constructor private, and use factory function design pattern that returns a result object indicating the failure.
  • For errno, you should be able to directly convert it into std::errc which is std::error_code compatible.
• Drop is_open method.
• Use move semantics without introducing unique pointers.

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.cpp`:
- Around line 32-34: The catch block in ReadOnlyMemoryMappedFile.cpp currently
reads errno during stack unwinding (catching TraceableException) which may be
stale; change the flow so FileDescriptor::OperationFailed captures errno at
throw-time (e.g., store the errno value inside the exception object or
ErrorCode_errno payload) and then, in the catch handler for TraceableException
in ReadOnlyMemoryMappedFile (and any similar handlers), use the stored errno
value from the exception (accessor on TraceableException or OperationFailed) to
construct the std::error_code instead of reading the global errno; update the
OperationFailed constructor to accept/capture ::errno and expose it via a method
or member that the catch site can call.

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.hpp`:
- Around line 40-46: The move-assignment operator for ReadOnlyMemoryMappedFile
currently overwrites m_data and m_buf_size without unmapping the existing
mapping, leaking the mmap; update operator=(ReadOnlyMemoryMappedFile&&) to first
check if m_data != nullptr and call munmap(m_data, m_buf_size) (or the class's
unmap helper) on the current object before taking ownership from rhs, then move
m_data and m_buf_size using std::exchange as before; ensure munmap is available
by adding `#include` <sys/mman.h> to the header or move the operator
implementation into the .cpp where munmap is already included.

In `@components/core/src/clp/streaming_compression/zstd/Decompressor.cpp`:
- Around line 178-192: The SPDLOG_ERROR call inside Decompressor::create (the
block that handles ReadOnlyMemoryMappedFile::create failure where
m_memory_mapped_file is set and file_view is obtained) uses the prefix
"ZstdDecompressor"; update that log message to use the consistent prefix
"streaming_compression::zstd::Decompressor" to match other logs in this file
(e.g., the messages near lines with SPDLOG_ERROR/SPDLOG_WARN at the top of
Decompressor) so all log entries reference
streaming_compression::zstd::Decompressor.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.hpp`:
- Around line 34-37: The [[nodiscard]] attribute on the deleted copy-assignment
operator is redundant; remove the attribute so the declaration reads simply
"auto operator=(ReadOnlyMemoryMappedFile const&) -> ReadOnlyMemoryMappedFile& =
delete;" (or even "ReadOnlyMemoryMappedFile& operator=(ReadOnlyMemoryMappedFile
const&) = delete;") next to the already-deleted copy constructor in the
ReadOnlyMemoryMappedFile class to clean up dead code.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@components/core/src/clp/ReadOnlyMemoryMappedFile.hpp`:
- Around line 43-53: The move-assignment operator in ReadOnlyMemoryMappedFile
uses munmap but the header lacks `#include` <sys/mman.h>, causing compilation when
included transitively; either add `#include` <sys/mman.h> to this header or move
the operator=(ReadOnlyMemoryMappedFile&&) implementation into the .cpp where
sys/mman.h is already included, and while updating, make the null check style
consistent by using nullptr != m_data (instead of m_data != nullptr) and keep
the uses of m_data and m_buf_size intact when calling munmap and exchanging
values from rhs.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

components/core/src/clp_s/ZstdDecompressor.cpp (1)

243-269: ⚠️ Potential issue | 🟡 Minor

m_input_type is set before create() — failed open leaves the object in a partially-initialized state.

If ReadOnlyMemoryMappedFile::create fails (line 251), m_input_type is already set to MemoryMappedCompressedFile (line 247), but the function returns ErrorCodeFailure without resetting it. A subsequent call to open() will throw ErrorCodeNotReady because m_input_type != NotInitialized. The caller must explicitly close() before retrying.

This same pattern exists in clp::streaming_compression::zstd::Decompressor::open (line 175 of that file), so it's a pre-existing convention. However, resetting m_input_type on the error path would make the API more forgiving:

Proposed fix (applies to both decompressors)

     if (result.has_error()) {
         SPDLOG_ERROR(
                 "ZstdDecompressor: Unable to memory map the compressed file with path: {}. Error: "
                 "{}.",
                 compressed_file_path.c_str(),
                 result.error().message()
         );
+        m_input_type = InputType::NotInitialized;
         return ErrorCodeFailure;
     }

[LinZhihao-723]

A small catch otherwise we should be good to go.

[LinZhihao-723]

For the PR title, how about:

feat(core): Adopt two-phase construction for `clp::ReadOnlyMemoryMappedFile`; Remove `boost::iostreams` dependency in clp-s.

• This is not just a refactor PR anymore. It changes the API and drops dependencies.
• The scope should be core.