Add support for Frame format #38
Conversation
WalkthroughThe PHP lz4 extension adds frame-format support: the bundled build now compiles Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ext as lz4_compress_frame
participant LZ4F as LZ4F_compressFrame
participant Output
rect rgb(240,248,255)
User->>ext: call(data, level, max_block_size, checksums)
ext->>ext: build framePrefs, determine blockSizeID & checksums
ext->>ext: allocate output buffer
ext->>LZ4F: compress with framePrefs
alt success
LZ4F-->>Output: compressed frame bytes
Output-->>User: return string
else error
LZ4F-->>ext: error code
ext-->>User: return false + warning
end
end
sequenceDiagram
participant User
participant ext as lz4_uncompress_frame
participant LZ4F as LZ4F_decompress
participant Output
rect rgb(245,255,240)
User->>ext: call(compressed_frame)
ext->>ext: parse frame info, determine initial output size
ext->>ext: allocate/delegate output buffer
ext->>LZ4F: decompress chunks
alt success
LZ4F-->>Output: decompressed data
Output-->>User: return string
else need_more_space
ext->>ext: resize buffer and retry
ext-->>User: return string or false on failure
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.m4(1 hunks)lz4.c(6 hunks)lz4.stub.php(1 hunks)tests/frame_001.phpt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lz4.c (1)
lz4.stub.php (2)
lz4_compress_frame(39-39)lz4_uncompress_frame(42-42)
lz4.stub.php (1)
lz4.c (2)
lz4_compress_frame(508-543)lz4_uncompress_frame(545-574)
🪛 PHPMD (2.15.0)
lz4.stub.php
39-39: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
39-39: Avoid unused parameters such as '$level'. (undefined)
(UnusedFormalParameter)
39-39: Avoid unused parameters such as '$max_block_size'. (undefined)
(UnusedFormalParameter)
39-39: Avoid unused parameters such as '$checksums'. (undefined)
(UnusedFormalParameter)
42-42: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (3)
config.m4 (1)
62-62: Bundled build now includes lz4frame.
Ensures the bundled build ships the frame symbols needed by the new PHP entry points.tests/frame_001.phpt (1)
20-27: Nice round-trip coverage.
Covers both big and small payloads and asserts deterministic output, which should keep regressions out.lz4.stub.php (1)
35-40: Docblock captures the frame options clearly.
Signature and parameter docs line up with the C implementation, so users know how to tune block size and checksum behavior.
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lz4.c(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lz4.c (1)
lz4.stub.php (2)
lz4_compress_frame(39-39)lz4_uncompress_frame(42-42)
🔇 Additional comments (5)
lz4.c (5)
42-42: LGTM!The new include, forward declarations, arginfo blocks, and function registrations follow the standard PHP extension patterns and are correctly structured.
Also applies to: 72-73, 87-96, 106-107
323-331: LGTM!The helper function correctly maps LZ4F block size IDs to their corresponding byte values.
360-370: LGTM!The buffer allocation, decompression loop, and cleanup logic are well-structured. The zero-progress check (lines 412-420) is good defensive programming.
Also applies to: 389-421, 424-430
533-547: LGTM!Line 538 correctly uses
RETURN_FALSEto bail out immediately on failure, properly addressing the past review comment. The return value handling and cleanup are correct.
566-578: LGTM!The function call, error handling, return value setup, and cleanup are all correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
36-172: Excellent documentation for the new frame API!The documentation is comprehensive and clearly explains the new frame-format functions, their parameters, and the constants. The distinction between block format and frame format is helpful for users.
If you'd like to address the minor grammar suggestions from static analysis, consider using hyphens for compound modifiers: "frame-level checksum" and "block-level checksum" (lines 144, 147-148).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(3 hunks)lz4.c(7 hunks)lz4.stub.php(1 hunks)tests/frame_002.phpt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lz4.c (1)
lz4.stub.php (4)
lz4_compress(66-66)lz4_uncompress(69-69)lz4_compress_frame(75-75)lz4_uncompress_frame(78-78)
lz4.stub.php (1)
lz4.c (4)
lz4_compress(445-483)lz4_uncompress(485-520)lz4_compress_frame(522-557)lz4_uncompress_frame(559-588)
🪛 LanguageTool
README.md
[grammar] ~144-~144: Use a hyphen to join words.
Context: .... * checksums Enable/disable frame level and block level checksums. Pass ...
(QB_NEW_EN_HYPHEN)
[grammar] ~144-~144: Use a hyphen to join words.
Context: ... Enable/disable frame level and block level checksums. Pass a bitwise combin...
(QB_NEW_EN_HYPHEN)
[grammar] ~147-~147: Use a hyphen to join words.
Context: ...ants: * LZ4_CHECKSUM_FRAME: frame level checksum * _LZ4_CHECKSUM_BLOCK...
(QB_NEW_EN_HYPHEN)
[grammar] ~148-~148: Use a hyphen to join words.
Context: ...ecksum * LZ4_CHECKSUM_BLOCK: block level checksum #### Return Values Retu...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🪛 PHPMD (2.15.0)
lz4.stub.php
66-66: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$level'. (undefined)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$extra'. (undefined)
(UnusedFormalParameter)
69-69: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
69-69: Avoid unused parameters such as '$maxsize'. (undefined)
(UnusedFormalParameter)
69-69: Avoid unused parameters such as '$offset'. (undefined)
(UnusedFormalParameter)
75-75: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
75-75: Avoid unused parameters such as '$level'. (undefined)
(UnusedFormalParameter)
75-75: Avoid unused parameters such as '$max_block_size'. (undefined)
(UnusedFormalParameter)
75-75: Avoid unused parameters such as '$checksums'. (undefined)
(UnusedFormalParameter)
78-78: Avoid unused parameters such as '$data'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (10)
tests/frame_002.phpt (1)
1-37: LGTM! Comprehensive test coverage.The test effectively validates the frame compression API with all parameters, testing both large and small data payloads. The round-trip verification using
strcmpand binary output validation ensure the implementation works correctly.lz4.stub.php (1)
29-78: LGTM! Stub definitions are correct.The new constants and function signatures are properly defined and match the C implementation in
lz4.c. The docblocks provide helpful parameter documentation.Note: PHPMD warnings about unused parameters are false positives—this is a stub file where the actual implementations are in C, so the parameters appear unused in the PHP context.
lz4.c (8)
42-42: LGTM!The
lz4frame.hinclude is necessary for the frame format API functions.
70-71: LGTM!The checksum flag constants are properly defined for bitwise operations, enabling users to combine frame and block checksums.
75-76: LGTM! Proper extension registration.The function declarations, argument info structures, function entries, and constant registrations all follow standard PHP extension patterns and correctly expose the new frame API.
Also applies to: 90-110, 121-126
288-331: LGTM! Past issues have been addressed.The compression function correctly:
- Initializes preferences before calculating the frame bound (addressing past review feedback)
- Uses
LZ4F_isErrorfor proper error detection with descriptive error messages- Validates block size parameters
- Handles memory allocation errors appropriately
The level validation was intentionally omitted to leverage the library's built-in sanitization, as discussed in previous review comments.
333-341: LGTM!The helper correctly maps frame block size IDs to actual byte sizes for buffer allocation. The bit shift calculations are accurate and the default case properly handles the 64KB size.
343-443: LGTM! Past issues have been addressed.The decompression function correctly:
- Checks
!tmpinstead of!*tmp, fixing the null pointer dereference (addressed in commit 6add51c)- Uses accurate error messages that reflect the actual operation
- Implements adaptive buffer reallocation for frames without a known content size
- Detects stall conditions to prevent infinite loops
- Properly cleans up resources on all error paths
The implementation is robust and handles both fixed-size and streaming decompression scenarios.
522-557: LGTM! Proper error handling.The function correctly uses
RETURN_FALSE(addressed in commit 71c6874), which immediately exits on failure, preventing the erroneous execution of the success path. Error messages are accurate and memory management follows PHP extension conventions.
559-588: LGTM! Correct error handling and naming.The function correctly:
- Uses the accurate function name in error messages (addressed in commit 6add51c)
- Exits immediately on failure with
RETURN_FALSE- Uses
free()instead ofefree()to match themalloc()allocation inphp_lz4_uncompress_frame
|
@cracksalad Please fix config.w32 because it causes errors during Windows builds. config.w32: - ADD_SOURCES("lz4/lib", "lz4.c lz4hc.c xxhash.c", "lz4", "lz4\\lib");
+ ADD_SOURCES("lz4/lib", "lz4.c lz4frame.c lz4hc.c xxhash.c", "lz4", "lz4\\lib"); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
README.md (1)
38-40: Optional: Consider code block style for consistency with linter.The markdown linter prefers indented code blocks over fenced blocks in this context. However, this is a minor style preference and the current fenced block style is perfectly readable and consistent with other code blocks in the documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(3 hunks)config.w32(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🔇 Additional comments (5)
config.w32 (1)
9-9: LGTM! Critical Windows build fix applied.This change adds
lz4frame.cto the build sources as requested by the maintainer to fix Windows builds. The addition is necessary to support the new frame format functions and prevents linking errors on Windows.README.md (4)
36-40: LGTM! Useful installation alternative documented.The PIE installation method is clearly documented and provides users with an alternative installation approach.
51-54: LGTM! Clear format distinction.The updated function list clearly distinguishes between block format and frame format operations, helping users choose the appropriate API.
115-152: Well-documented frame compression API.The documentation for
lz4_compress_frameis comprehensive and covers all parameters, constants, and usage patterns clearly. Users will have sufficient information to use the frame format API effectively.
159-172: Clear and concise decompression documentation.The documentation for
lz4_uncompress_frameis straightforward and appropriate for the simpler decompression API.
@kjdev Thanks for reviewing! This issue should be fixed now. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
64-64: Typo corrections applied.The "Pameters" typos flagged in prior reviews have been corrected to "Parameters" in both locations. ✓
Also applies to: 92-92
🧹 Nitpick comments (1)
README.md (1)
36-40: Markdown code-block style inconsistency flagged by linter.The fenced code block here is consistent with existing examples in the file (lines 22–24), but markdownlint (MD046) prefers indented code blocks. This is a minor style issue; consider aligning all code blocks in the file to a single style (either all fenced or all indented) to resolve the warning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
38-38: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🔇 Additional comments (2)
README.md (2)
51-54: Clear documentation of block vs. frame format functions.The function list now explicitly distinguishes block-format and frame-format variants, making it easier for users to select the appropriate function for their use case.
111-172: Well-documented frame-format APIs.The new documentation for
lz4_compress_frameandlz4_uncompress_framefollows the established pattern, with comprehensive parameter descriptions including block-size constants and checksum options. The structure and clarity are consistent with the existing function documentation.
|
Thanks. |
|
@kjdev thank you very much for merging and creating a new release that fast! |
2 participants
Added two exported functions:
lz4_compress_framelz4_uncompress_frameBoth functions work similar to the already existing ones, except they handle the LZ4 Frame format instead of the Block format. The difference is of advantage if you need checksums or magic bytes.
Also added a pretty basic test case.
I look forward to reading your thoughts on this.
See also: #34
Summary by CodeRabbit
New Features
Tests
Documentation
Chores