refactor(core): Remove `clp::Array` and migrate to `ystdlib::containers::Array`. by Bill-hbrhbr · Pull Request #712 · y-scope/clp

Bill-hbrhbr · 2025-02-03T00:31:03Z

Description

As title.

This PR also cleans up the header includes for clp::Array. It was previously a standalone file in core-clp, included via fragile relative paths, and used by projects outside of core-clp, leading to a convoluted structure.
It is now correctly included as a library class from ystdlib::containers, with unit tests relocated to ystdlib-cpp. For nuanced changes to the original Array class, see the migration PR in ystdlib-cpp.

Checklist

The PR satisfies the contribution guidelines.
This isn't a breaking change.
Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Existing code that uses the original clp::Array pass their unit tests after the change.

Summary by CodeRabbit

New Features
- Integrated a standardized array library to enhance data handling across various functionalities, including compression and network operations.
Refactor
- Transitioned from legacy array implementations to the new library for improved consistency and performance.
Chores
- Updated build configurations and dependencies to support the new library integration.
Tests
- Revised test cases to confirm compatibility with the upgraded array functionality.

coderabbitai · 2025-02-03T00:31:10Z

Walkthrough

This pull request refactors the project’s array handling by replacing the local clp::Array implementation with the standardized ystdlib::containers::Array. The changes include removing the original Array.hpp file and its associated tests, updating include directives in multiple source and header files, and modifying member variable types and initializations accordingly. In addition, several CMake configuration files have been updated to link against the new ystdlib::containers library. There are no modifications to the core control flow or external functionality.

Changes

File(s)	Change Summary
`components/core/CMakeLists.txt`	Removed `src/clp/Array.hpp` and `tests/test-Array.cpp` from unit test sources; added `ystdlib::containers` to the `target_link_libraries(unitTest ...)`.
`components/core/src/clp/{clg, clo, clp, make_dictionaries_readable}/CMakeLists.txt`, `components/core/src/clp_s/{, indexer}/CMakeLists.txt`	Added `ystdlib::containers` dependency in the `target_link_libraries` sections for the respective executables.
`components/core/src/clp/Array.hpp`	Completely removed the file containing the local Array implementation.
`components/core/src/clp/NetworkReader.hpp`	Replaced the include of `"Array.hpp"` with `<ystdlib/containers/Array.hpp>`; updated the type of `m_buffer_pool` to use `ystdlib::containers::Array<char>`.
`components/core/src/clp/streaming_compression/{lzma/Compressor.hpp, zstd/Compressor.hpp, zstd/Decompressor.hpp}`	Updated include directives to `<ystdlib/containers/Array.hpp>`, modified member variable types, and adjusted constructor initialization (in lzma Compressor).
`components/core/src/clp/streaming_archive/ArchiveMetadata.cpp`	Changed the include of the local `Array.hpp` to `<ystdlib/containers/Array.hpp>` and modified the buffer instantiation from `clp::Array<char>{…}` to `ystdlib::containers::Array<char>(…)`.
`components/core/src/clp/streaming_compression/zstd/{Compressor.cpp, Decompressor.cpp}`	Refined initialization of buffer member variables to utilise `ystdlib::containers::Array`.
`components/core/tests/test-Array.cpp`	Removed the entire test file for the local Array implementation.
`components/core/tests/{test-FileDescriptorReader.cpp, test-NetworkReader.cpp, test-StreamingCompression.cpp}`	Updated header inclusions and Array instantiations to transition from `clp::Array` to `ystdlib::containers::Array`.

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)

We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d988118 and 30c27a8.

📒 Files selected for processing (18)

components/core/CMakeLists.txt (1 hunks)
components/core/src/clp/NetworkReader.hpp (2 hunks)
components/core/src/clp/clg/CMakeLists.txt (1 hunks)
components/core/src/clp/clo/CMakeLists.txt (1 hunks)
components/core/src/clp/clp/CMakeLists.txt (1 hunks)
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1 hunks)
components/core/src/clp/streaming_archive/ArchiveMetadata.cpp (2 hunks)
components/core/src/clp/streaming_compression/lzma/Compressor.hpp (3 hunks)
components/core/src/clp/streaming_compression/zstd/Compressor.cpp (1 hunks)
components/core/src/clp/streaming_compression/zstd/Compressor.hpp (2 hunks)
components/core/src/clp/streaming_compression/zstd/Decompressor.cpp (1 hunks)
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (2 hunks)
components/core/src/clp_s/CMakeLists.txt (1 hunks)
components/core/src/clp_s/indexer/CMakeLists.txt (1 hunks)
components/core/tests/test-Array.cpp (0 hunks)
components/core/tests/test-FileDescriptorReader.cpp (2 hunks)
components/core/tests/test-NetworkReader.cpp (2 hunks)
components/core/tests/test-StreamingCompression.cpp (3 hunks)

💤 Files with no reviewable changes (1)

components/core/tests/test-Array.cpp

✅ Files skipped from review due to trivial changes (1)

components/core/src/clp/streaming_compression/zstd/Decompressor.cpp

🚧 Files skipped from review as they are similar to previous changes (13)

components/core/src/clp/clo/CMakeLists.txt
components/core/src/clp_s/indexer/CMakeLists.txt
components/core/src/clp/clg/CMakeLists.txt
components/core/tests/test-FileDescriptorReader.cpp
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt
components/core/tests/test-NetworkReader.cpp
components/core/src/clp/clp/CMakeLists.txt
components/core/src/clp_s/CMakeLists.txt
components/core/src/clp/streaming_compression/zstd/Compressor.hpp
components/core/src/clp/NetworkReader.hpp
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp
components/core/tests/test-StreamingCompression.cpp
components/core/src/clp/streaming_compression/lzma/Compressor.hpp

🧰 Additional context used

📓 Path-based instructions (1)

`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.

**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Prefer false == <expression> rather than !<expression>.

components/core/src/clp/streaming_compression/zstd/Compressor.cpp
components/core/src/clp/streaming_archive/ArchiveMetadata.cpp

🔇 Additional comments (4)

components/core/src/clp/streaming_compression/zstd/Compressor.cpp (1)

14-15: Implementation looks correct and adheres to coding standards

The initialization of the new m_compressed_stream_block_buffer member using ZSTD_CStreamOutSize() is appropriate, providing the right buffer size for Zstd compression operations. The member initialization list properly sets up the compression stream block structure with the buffer. This change successfully transitions from the local Array implementation to ystdlib::containers::Array as intended by the refactoring.

components/core/CMakeLists.txt (1)

664-664: Successfully added dependency on ystdlib::containers

The addition of ystdlib::containers to the target_link_libraries section properly supports the refactoring of Array from clp to ystdlib.
components/core/src/clp/streaming_archive/ArchiveMetadata.cpp (2)

6-6: Appropriate header inclusion for the refactored Array class

The inclusion of the ystdlib containers Array header replaces the previously used clp Array header, aligning with the refactoring objective.

29-29:
Details

✅ Verification successful

Correctly transitioned to ystdlib::containers::Array implementation

The Array instantiation has been properly updated to use the new namespace and constructor syntax. Note that the initialization has changed from using curly braces to parentheses, which matches the expected constructor signature for the refactored class.

🏁 Script executed:
#!/bin/bash
# Check for any other instances of clp::Array that might have been missed
rg -g "*.{cpp,hpp}" "clp::Array" --no-heading
Length of output: 45
Verification Complete: Array Instantiation Refactoring Confirmed

The instantiation of the Array using
ystdlib::containers::Array<char> buf(static_cast<size_t>(file_stat.st_size));
in components/core/src/clp/streaming_archive/ArchiveMetadata.cpp (line 29) is correctly updated. A repository-wide search for any remaining instances of clp::Array returned no matches, confirming that the refactor is complete.

✨ Finishing Touches

📝 Generate Docstrings

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai plan to trigger planning for file edits and PR creation.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (2)

components/core/tests/test-Array.cpp (1)
13-13: Update test case tags to reflect new namespace.

The test case tags still reference "clp::Array" but should be updated to "ystdlib::Array" to match the new namespace.
-TEST_CASE("array_fundamental", "[clp::Array]") {
+TEST_CASE("array_fundamental", "[ystdlib::Array]") {

-TEST_CASE("array_default_initializable", "[clp::Array]") {
+TEST_CASE("array_default_initializable", "[ystdlib::Array]") {
Also applies to: 45-45
.gitmodules (1)

32-34: Submodule Addition: New ystdlib-cpp Reference
The new submodule entry for ystdlib-cpp is correctly added, providing the necessary URL and path for the library. This aligns well with the overall objective of refactoring the Array functionality to be sourced from an external module.

It might be worth considering pinning a specific commit or tag for stability in future builds.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f782ca5 and d988118.

📒 Files selected for processing (18)

.gitmodules (1 hunks)
components/core/CMakeLists.txt (2 hunks)
components/core/src/clp/Array.hpp (0 hunks)
components/core/src/clp/NetworkReader.hpp (2 hunks)
components/core/src/clp/clg/CMakeLists.txt (1 hunks)
components/core/src/clp/clo/CMakeLists.txt (1 hunks)
components/core/src/clp/clp/CMakeLists.txt (1 hunks)
components/core/src/clp/make_dictionaries_readable/CMakeLists.txt (1 hunks)
components/core/src/clp/streaming_compression/lzma/Compressor.hpp (2 hunks)
components/core/src/clp/streaming_compression/zstd/Compressor.hpp (2 hunks)
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (2 hunks)
components/core/src/clp_s/CMakeLists.txt (1 hunks)
components/core/src/clp_s/indexer/CMakeLists.txt (1 hunks)
components/core/submodules/ystdlib-cpp (1 hunks)
components/core/tests/test-Array.cpp (1 hunks)
components/core/tests/test-FileDescriptorReader.cpp (2 hunks)
components/core/tests/test-NetworkReader.cpp (2 hunks)
components/core/tests/test-StreamingCompression.cpp (2 hunks)

💤 Files with no reviewable changes (1)

components/core/src/clp/Array.hpp

✅ Files skipped from review due to trivial changes (1)

components/core/submodules/ystdlib-cpp

🧰 Additional context used

📓 Path-based instructions (8)

components/core/tests/test-FileDescriptorReader.cpp (1)