Skip to content

Conversation

@jonathan-imanu
Copy link
Contributor

@jonathan-imanu jonathan-imanu commented Feb 11, 2026

Description

This PR addresses #1922. It addresses raw new usage in two places:

  • We now use std::make_unique instead of raw new's to instantiate column writers (e.g., TimestampColumnWriter, ClpStringColumnWriter, etc.) in components/core/src/clp_s/ArchiveWriter.cpp.
  • In append_message we replace std::map<int32_t, SchemaWriter*> with std::map<int32_t, std::unique_ptr<SchemaWriter>>.

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

  • Unit and Integration tests

Summary by CodeRabbit

  • Refactor
    • Improved internal memory and ownership handling to increase stability and reduce risk of memory-related issues.
    • Updated component internals to use modern ownership semantics, simplifying resource lifecycle management.
    • No change to public interfaces or external behaviour; existing functionality and outputs remain unchanged.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 11, 2026

Walkthrough

The pull request refactors memory management in ArchiveWriter and SchemaWriter: raw pointers for SchemaWriter and BaseColumnWriter are replaced with std::unique_ptr, ownership is transferred via make_unique/move, and manual delete/destructor cleanup is removed in favour of RAII.

Changes

Cohort / File(s) Summary
ArchiveWriter Memory Management
components/core/src/clp_s/ArchiveWriter.hpp, components/core/src/clp_s/ArchiveWriter.cpp
Replaced std::map<int32_t, SchemaWriter*> with std::map<int32_t, std::unique_ptr<SchemaWriter>>. Initialization uses std::make_unique and emplace; lookups use iterator (it->second) and explicit delete calls were removed. Added <memory> include.
SchemaWriter Memory Management
components/core/src/clp_s/SchemaWriter.hpp, components/core/src/clp_s/SchemaWriter.cpp
Changed append_column to accept std::unique_ptr<BaseColumnWriter> and store columns in std::vector<std::unique_ptr<BaseColumnWriter>> (removed unordered columns). Destructor removed/ defaulted; manual deletion logic eliminated. Added <memory> include and updated ownership transfer via std::move.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% 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
Title check ✅ Passed The title accurately describes the main change: refactoring ArchiveWriter to replace raw pointer allocations with smart pointers (std::unique_ptr and std::make_unique), which is the primary focus across all modified files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ 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

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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

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

@jonathan-imanu jonathan-imanu changed the title refactor(clp-s): Replace raw new in ArchiveWriter with smart pointers (fixes [#1922](https://github.com/y-scope/clp/issues/1922)) refactor(clp-s): Replace raw new in ArchiveWriter with smart pointers (fixes #1922) Feb 11, 2026
@jonathan-imanu jonathan-imanu changed the title refactor(clp-s): Replace raw new in ArchiveWriter with smart pointers (fixes #1922) refactor(clp-s): Replace raw new in ArchiveWriter with smart pointers (fixes #1922). Feb 11, 2026
@jonathan-imanu jonathan-imanu marked this pull request as ready for review February 11, 2026 15:06
@jonathan-imanu jonathan-imanu requested review from a team and gibber9809 as code owners February 11, 2026 15:06
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ArchiveWriter.cpp (1)

252-266: 🧹 Nitpick | 🔵 Trivial

SchemaWriter itself is still managed with raw new/delete.

SchemaWriter is allocated with new on line 259, stored as a raw pointer in m_id_to_schema_writer, and manually deleted on line 430. This leaves dangling pointers in the map between the delete calls in store_tables() and the clear() on line 125. Consider converting m_id_to_schema_writer to store std::unique_ptr<SchemaWriter> as a follow-up to complete the smart pointer migration.

@jonathan-imanu jonathan-imanu changed the title refactor(clp-s): Replace raw new in ArchiveWriter with smart pointers (fixes #1922). refactor(clp-s): Replace usages of new in ArchiveWriter with smart pointers (fixes #1922). Feb 11, 2026
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ArchiveWriter.hpp (1)

1-11: 🧹 Nitpick | 🔵 Trivial

Consider adding #include <memory> directly.

This header now uses std::unique_ptr directly in the member declaration on line 350, but relies on <memory> being transitively included via SchemaWriter.hpp. Adding a direct #include <memory> would make the dependency explicit and prevent breakage if SchemaWriter.hpp ever stops including it.

Proposed fix
 `#include` <optional>
+#include <memory>
 `#include` <string>

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ArchiveWriter.hpp (1)

279-279: 🧹 Nitpick | 🔵 Trivial

Consider taking SchemaWriter& instead of SchemaWriter* for the non-owning parameter.

The function is always called with a valid (non-null) pointer obtained from unique_ptr::get() at line 257. A reference parameter would better express the non-null contract and follow modern C++ conventions for non-owning parameters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant