refactor(clp::streaming_compression): Use clp::Array in zstd decompressor to replace C-style arrays. - #699
Conversation
WalkthroughThe pull request focuses on modernizing the Zstandard decompression implementation in the Changes
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/src/clp/streaming_compression/zstd/Decompressor.cpp (1)
52-54: Consider revising the error messageWhile the validation is good, the error code
ErrorCode_Corruptmight be misleading. Consider usingErrorCode_NotInitorErrorCode_BadStateto better reflect that the buffer hasn't been initialized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/core/src/clp/streaming_compression/zstd/Decompressor.cpp(4 hunks)components/core/src/clp/streaming_compression/zstd/Decompressor.hpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
components/core/src/clp/streaming_compression/zstd/Decompressor.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: build (macos-latest)
- GitHub Check: build-macos (macos-13, true)
- GitHub Check: lint-check (macos-latest)
🔇 Additional comments (7)
components/core/src/clp/streaming_compression/zstd/Decompressor.cpp (5)
12-15: Well-structured constructor initialization!The member initializer list properly initializes the decompression stream and buffer, following C++ best practices. The buffer size is correctly determined using
ZSTD_DStreamOutSize().
55-58: LGTM! Modern C++ buffer handlingThe code properly uses
std::optionaland modern buffer access methods, improving safety and readability.
122-122: LGTM! Safe buffer access in seek operationThe code properly uses modern buffer access methods, maintaining consistency with the modernization efforts.
Also applies to: 126-126
170-170: LGTM! Efficient buffer initializationThe code efficiently uses
emplacefor buffer initialization and properly sets up the compressed stream block.Also applies to: 173-173
Line range hint
1-249: LGTM! Consistent modernization throughout the fileThe code demonstrates consistent use of modern C++ features and proper error handling patterns. The boolean comparisons correctly follow the coding guideline of using
false == <expression>.components/core/src/clp/streaming_compression/zstd/Decompressor.hpp (2)
5-5: LGTM! The new includes support the modernized buffer management.The addition of
<optional>and"../../Array.hpp"headers properly supports the transition from raw pointers to modern C++ types.Also applies to: 10-10
133-134: Great modernization of buffer management!The changes from raw pointers to modern C++ types (
Array<char>andstd::optional) improve safety and maintainability:
- Automatic memory management
- Built-in bounds checking
- Explicit optional semantics
- Elimination of manual size tracking
Consider documenting any performance requirements, as
Arraybounds checking andstd::optionalmight introduce minor overhead compared to raw pointers.Let's verify the performance characteristics of the
Arrayclass:Also applies to: 140-141
✅ Verification successful
Performance concerns are addressed in the Array implementation
The Array class is well-optimized with:
- Optional bounds checking (only in .at() methods)
- Direct buffer access via data() for performance-critical code
- Zero-cost abstractions for memory management
- Efficient initialization using memset for fundamental types
The change to Array maintains good performance while adding safety features that can be bypassed when needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if Array has debug-only bounds checking # Look for bounds checking conditionals or assertions in Array implementation ast-grep --pattern 'assert($_)' '../../Array.hpp'Length of output: 112
Script:
#!/bin/bash # Find Array.hpp file fd -t f "Array.hpp" # Once found, examine its content rg -l "class Array" --type cppLength of output: 218
Script:
#!/bin/bash # Examine Array implementation cat components/core/src/clp/Array.hpp # Look for bounds checking and assertions rg "assert|CHECK|ASSERT" components/core/src/clp/Array.hpp # Look for performance-related attributes or inline hints rg "inline|noexcept|constexpr" components/core/src/clp/Array.hppLength of output: 3358
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
Minor comment, but otherwise looks good.
| if (false == m_read_buffer.has_value()) { | ||
| throw OperationFailed(ErrorCode_Corrupt, __FILENAME__, __LINE__); | ||
| } | ||
| auto& read_buffer{m_read_buffer.value()}; |
There was a problem hiding this comment.
Is this check necessary? Seems like the open/close flow will force m_read_buffer.has_value() to always be true here.
There was a problem hiding this comment.
This is to:
- Prevent future developers from breaking the assumption about the current
open/closeflow (especially we don't have unit tests for this code path atm) - Silent clang-tidy warnings
If any later benchmark shows this becomes the bottleneck, we can remove it by manually silent clang-tidy for line 55.
gibber9809
left a comment
gibber9809
left a comment
There was a problem hiding this comment.
LGTM. PR title seems fine as well.
…ressor to replace C-style arrays. (y-scope#699)
2 participants
Description
This PR uses
clp::Arrayto replace C-style array (backed using a unique pointer) to modernize the implementation so that it matches our latest coding standard.Validation performed
Summary by CodeRabbit
Refactor
Bug Fixes
The changes focus on improving the internal implementation of the Zstandard decompression mechanism, making the code more robust and maintainable.