Fix/streaming uncompress dict #81
Conversation
|
""" WalkthroughA new test file was added to verify streaming compression and decompression with dictionaries using the zstd PHP extension. Additionally, the Changes
Sequence Diagram(s)sequenceDiagram
participant TestScript
participant ZstdExtension
participant Libzstd
TestScript->>ZstdExtension: Compress data with dictionary (streaming)
ZstdExtension->>Libzstd: Compress using provided dictionary
Libzstd-->>ZstdExtension: Compressed data
ZstdExtension-->>TestScript: Write compressed data to file
TestScript->>ZstdExtension: Decompress data with dictionary (streaming)
ZstdExtension->>Libzstd: Decompress (handle unknown frame size)
Libzstd-->>ZstdExtension: Decompressed data (streaming)
ZstdExtension-->>TestScript: Return decompressed data
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (17)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 review |
✅ Actions performedReview triggered.
|
![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 (1)
zstd.c (1)
582-596: Consider preventing potential double-free issues.While the current code flow is correct, it would be safer to set
output = NULLafterefree(output)to prevent any potential double-free issues if the code is modified in the future.Apply this diff to improve safety:
if (ZSTD_IS_ERROR(result)) { zend_string_efree(output); + output = NULL; ZSTD_WARNING("%s", ZSTD_getErrorName(result)); RETVAL_FALSE; } else if (result != size) { zend_string_efree(output); + output = NULL; ZSTD_WARNING("failed to uncompress"); RETVAL_FALSE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/dictionary_02.phpt(1 hunks)zstd.c(1 hunks)
🔇 Additional comments (5)
zstd.c (4)
546-550: LGTM! Well-structured variable declarations.The new variables are appropriately typed and necessary for implementing streaming decompression support.
557-566: Good implementation of frame content size handling.The three-way logic correctly handles all possible scenarios:
- Empty frames (size == 0)
- Invalid frames (ZSTD_CONTENTSIZE_ERROR)
- Streaming frames (ZSTD_CONTENTSIZE_UNKNOWN)
The initial buffer size estimation for streaming mode is reasonable.
568-578: Excellent error handling and resource management.The code properly creates the decompression context and dictionary with appropriate error messages and cleanup on failure.
597-636: Excellent streaming decompression implementation.The streaming mode correctly:
- Initializes input/output buffers
- References the dictionary using the streaming API
- Handles dynamic buffer expansion
- Includes comprehensive error handling with proper resource cleanup
- Breaks the loop when decompression is complete (result == 0)
tests/dictionary_02.phpt (1)
9-10:✅ Verification successful
Verify that required test data files exist.
The test depends on
data.incanddata.dicfiles. Please ensure these files are present in the test directory.Run the following script to verify the test data files:
🏁 Script executed:
#!/bin/bash # Description: Verify that required test data files exist for the dictionary test. # Test: Check if data.inc exists fd -t f "data.inc" tests/ # Test: Check if data.dic exists fd -t f "data.dic" tests/ # Test: List all dictionary-related test files to understand the test structure fd -t f "dictionary" tests/Length of output: 181
Required test data files verified
Both
tests/data.incandtests/data.dicare present in thetests/directory, satisfying the test’s dependencies. No further action needed.
kjdev
force-pushed
the
fix/streaming-uncompress-dict
branch
from
8db8932 to
8ed8865
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit
New Features
Bug Fixes