feat(core-clp): add LZMA Decompressor from closed source with minimal changes to style and functionality.
#703
Conversation
WalkthroughThis pull request introduces LZMA decompression functionality to the streaming compression module. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Decompressor
participant LZMALibrary
User->>Decompressor: open(compressed_data)
Decompressor->>LZMALibrary: init_decoder
User->>Decompressor: try_read()
Decompressor->>LZMALibrary: Decompress data
LZMALibrary-->>Decompressor: Return decompressed data
Decompressor-->>User: Provide decompressed data
Possibly related PRs
Suggested reviewers
✨ 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 (
|
Bill-hbrhbr
changed the title
Decompressor from closed source with minimal changes to style and functionality.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
components/core/src/clp/streaming_compression/lzma/Decompressor.hpp (2)
8-10: Consider removing the unused zlib header.
It seems that<zlib.h>is not used in this file. Removing it could reduce clutter.
16-16: Confirm necessity of including “FileReader.hpp”.
The class referencesReaderInterface, soFileReader.hppmight be superfluous.components/core/src/clp/streaming_compression/lzma/Decompressor.cpp (1)
299-299: Adhere to the code guideline of usingfalse == expr.
Instead of using!m_memory_mapped_compressed_file.is_open(), apply the preferred pattern:-if (!m_memory_mapped_compressed_file.is_open()) { +if (false == m_memory_mapped_compressed_file.is_open()) {components/core/tests/test-StreamingCompression.cpp (1)
137-138: Add edge-case or corruption tests.
This test covers basic LZMA decompression well. Adding tests for partially corrupted data or zero-length inputs would increase confidence in robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
components/core/CMakeLists.txt(1 hunks)components/core/src/clp/streaming_compression/lzma/Decompressor.cpp(1 hunks)components/core/src/clp/streaming_compression/lzma/Decompressor.hpp(1 hunks)components/core/tests/test-StreamingCompression.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/core/tests/test-StreamingCompression.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/lzma/Decompressor.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/streaming_compression/lzma/Decompressor.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
🪛 Cppcheck (2.10-2)
components/core/src/clp/streaming_compression/lzma/Decompressor.cpp
[error] 41-41: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std
(rethrowNoCurrentException)
[error] 45-45: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std
(rethrowNoCurrentException)
🔇 Additional comments (4)
components/core/src/clp/streaming_compression/lzma/Decompressor.hpp (1)
21-34: Class design looks appropriate for an LZMA decompressor.
The custom exception and the blocked copy/move constructors make sense. This looks good.components/core/src/clp/streaming_compression/lzma/Decompressor.cpp (1)
66-70: Validate the early EOF logic.
Whenm_input_typeis notFile, the code concludes there is no more data available. Confirm that partial decompression in other sources is impossible.components/core/tests/test-StreamingCompression.cpp (1)
21-21: Good job adding the correct header.
Including the LZMA decompressor header in this test file ensures canonic coverage.components/core/CMakeLists.txt (1)
543-544: LGTM! The LZMA decompressor files are properly integrated.The new source files are correctly added to the unit test build configuration, and the project already has proper LZMA library setup.
Let's verify that the unit tests for the LZMA decompressor are properly configured:
✅ Verification successful
LZMA decompressor is properly tested and integrated
The test file includes comprehensive test cases for the LZMA decompressor, verifying:
- Correct decompression of data
- Handling of various chunk sizes
- Data integrity validation
- Proper cleanup of resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify LZMA decompressor test configuration # Check if the test file contains LZMA decompressor tests rg -l "TEST.*LZMA.*Decompressor" "tests/test-StreamingCompression.cpp" # Check if the LZMA library is properly linked rg "LIBLZMA" "components/core/CMakeLists.txt"Length of output: 507
Script:
#!/bin/bash # Find the test file location echo "=== Searching for streaming compression test files ===" fd -t f -e cpp -e cxx "StreamingCompression|streaming.*compression" --ignore-case echo -e "\n=== Searching for files containing LZMA test cases ===" rg -l "TEST.*LZMA" -g "*.cpp" -g "*.cxx"Length of output: 422
Script:
#!/bin/bash # Examine the streaming compression test file echo "=== Streaming compression test content ===" cat components/core/tests/test-StreamingCompression.cpp echo -e "\n=== Searching for any compression test patterns ===" rg "TEST.*Decompress" components/core/tests/test-StreamingCompression.cppLength of output: 5782
| if (num_bytes_read != num_bytes_to_read) { | ||
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | ||
| throw; | ||
| } | ||
| if (errorcode != ErrorCode_Success) { | ||
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Replace 'throw;' with a specific exception throw.
There is no active exception to rethrow here, which can lead to undefined behaviour. Instead, consider throwing OperationFailed with a descriptive message:
- throw;
+ throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__);Similarly, do the same for the second occurrence of throw; in this block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (num_bytes_read != num_bytes_to_read) { | |
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | |
| throw; | |
| } | |
| if (errorcode != ErrorCode_Success) { | |
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | |
| throw; | |
| } | |
| if (num_bytes_read != num_bytes_to_read) { | |
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | |
| throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__); | |
| } | |
| if (errorcode != ErrorCode_Success) { | |
| SPDLOG_ERROR("FAILED TO READ EXACTLY {} bytes", num_bytes_to_read); | |
| throw OperationFailed(ErrorCode_Failure, __FILENAME__, __LINE__); | |
| } |
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 41-41: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std
(rethrowNoCurrentException)
[error] 45-45: Rethrowing current exception with 'throw;', it seems there is no current exception to rethrow. If there is no current exception this calls std
(rethrowNoCurrentException)
Description
Paste LZMA decompressor interface and implementation directly from
clp-privatewith the following changes:clp::to header guard and namespace.FileReadertoReaderInterface.Here's the diff between two versions. Refactoring and modernization of the code will be addressed in future PRs.
Validation performed
Summary by CodeRabbit
New Features
Decompressorclass for handling LZMA-compressed dataTests