Add tests for crc32_fold_copy functions#2005
Conversation
WalkthroughAdds three public crc32_fold function-pointer typedefs, extracts CRC-32 test vectors to a new header, removes inline test vectors from test_crc32.cc, adds parameterized fold-copy unit tests that use the new typedefs, and updates test CMake to include the new test file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Unit Test
participant Impl as crc32_fold impl
participant Buf as Buffer (src/dst)
Note left of Test: iterate crc32_tests
Test->>Impl: crc32_fold_reset(&state) --- (crc32_fold_reset_func)
Test->>Buf: prepare src data
Test->>Impl: crc32_fold_copy(&state, dst, src, len) --- (crc32_fold_copy_func)
Impl-->>Buf: copy bytes to dst
Test->>Impl: crc32_fold_final(&state) --- (crc32_fold_final_func)
Impl-->>Test: final CRC
Test->>Test: assert(final CRC == expected && dst == src)
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/test_crc32_fold_copy.cc (1)
22-44: Use consistent boolean operator syntax.Line 28 uses the
andkeyword instead of&&. While valid C++, this is inconsistent with the rest of the codebase which uses&&.Apply this diff:
- if(onlyzero and params.crc != 0) { + if(onlyzero && params.crc != 0) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/CMakeLists.txttest/test_crc32.ccarch/generic/generic_functions.htest/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/test_crc32.ccarch/generic/generic_functions.htest/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/test_crc32.cctest/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/test_crc32.cctest/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-06-18T19:28:32.987Z
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.
Applied to files:
arch/generic/generic_functions.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
arch/generic/generic_functions.htest/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
🧬 Code graph analysis (2)
arch/generic/generic_functions.h (1)
crc32.h (1)
crc32_fold_s(20-23)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🪛 GitHub Actions: Lint
test/test_crc32_fold_copy.cc
[error] 82-82: New blank line at end of file detected. Command failed: 'git diff --color --check 780838b -- './' ':!.patch'' (exit code 2).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
- GitHub Check: macOS GCC UBSAN (ARM64)
🔇 Additional comments (8)
test/CMakeLists.txt (1)
186-186: LGTM!The new test file is correctly added to the test suite under the appropriate conditional compilation guard.
arch/generic/generic_functions.h (1)
15-17: LGTM!The new function pointer typedefs for CRC32 fold operations are well-defined and consistent with existing patterns in the codebase. The signatures align with the implementations in
arch/generic/crc32_fold_c.c.test/test_crc32.cc (2)
13-18: LGTM!The refactoring to use external test data from
crc32_test_strings_p.himproves code organization and enables test data reuse across multiple test files.
77-77: LGTM!Correctly updated to use the externalized
crc32_testsdata source.test/crc32_test_strings_p.h (3)
1-12: LGTM!The buffer setup with proper alignment and deterministic initialization is well-structured. The use of a setup function called during static initialization is unconventional but valid and ensures the buffer is ready for use in test data.
14-19: LGTM!The
crc32_teststruct clearly defines the test parameters needed for CRC32 validation.
22-178: LGTM!The test data array provides comprehensive coverage including edge cases (NULL pointers, zero lengths, various string lengths, and the full 32KB buffer). This will enable thorough testing of CRC32 implementations.
test/test_crc32_fold_copy.cc (1)
71-72: Consider verifying the 16-byte minimum requirement.The comment questions whether pclmul functions truly require a 16-byte minimum. According to the PR objectives, the author observed that their tests worked with no minimum despite documentation claiming a 16-byte requirement. This might be worth investigating further to determine if the minimum can be safely reduced or if the current setting is conservative but correct.
Based on the PR objectives, you mentioned that the tests worked without the 16-byte minimum. Would you like to verify this behavior with additional testing or documentation review to determine if this requirement is truly necessary?
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2005 +/- ##
===========================================
- Coverage 83.21% 81.59% -1.62%
===========================================
Files 160 162 +2
Lines 12955 12823 -132
Branches 3145 3161 +16
===========================================
- Hits 10780 10463 -317
- Misses 1138 1299 +161
- Partials 1037 1061 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e7986a7 to
94eb051
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
test/test_crc32_fold_copy.cc (2)
23-23: Verify stack allocation safety.The aligned stack allocation of
crc32_foldstruct may cause issues if it's not the first variable allocated in the current scope, as flagged in a previous review.Based on learnings
82-83: Verify no trailing blank line at end of file.A previous review flagged a trailing blank line at the end of this file. Ensure the file ends immediately after the final
#endifwith only a single newline terminator.
🧹 Nitpick comments (2)
test/crc32_test_strings_p.h (1)
3-10: Consider usingsize_tfor the loop counter.Using
size_tinstead ofintfor array indexing is more idiomatic and avoids potential issues with negative values or integer overflow.Apply this diff:
static uint8_t* crc32_setup_buf() { - for (int i = 0; i < 32768; ++i) { + for (size_t i = 0; i < 32768; ++i) { uint8_t ic = (uint8_t)(i % 256); crc32_fullwin_buf[i] = ic; }test/test_crc32_fold_copy.cc (1)
28-30: Prefer&&overandfor consistency.While
andis a valid C++ alternative operator, using&&is more consistent with the rest of the codebase.Apply this diff:
- if(onlyzero and params.crc != 0) { + if(onlyzero && params.crc != 0) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- arch/generic/generic_functions.h
- test/test_crc32.cc
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/test_crc32_fold_copy.cctest/crc32_test_strings_p.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_crc32_fold_copy.cctest/crc32_test_strings_p.h
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/test_crc32_fold_copy.cctest/crc32_test_strings_p.h
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/test_crc32_fold_copy.cctest/crc32_test_strings_p.h
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/test_crc32_fold_copy.cctest/crc32_test_strings_p.h
🧬 Code graph analysis (2)
test/CMakeLists.txt (1)
test/benchmarks/benchmark_crc32.cc (1)
crc32(21-48)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🔇 Additional comments (3)
test/CMakeLists.txt (1)
186-186: LGTM!The test file addition follows the established pattern and includes a clear comment describing the test scope.
test/test_crc32_fold_copy.cc (2)
49-61: LGTM!The test suite instantiation and macro definition are correctly implemented. The generic test invocation provides all required parameters with appropriate values.
63-82: Conservative approach to pclmul minimum length is appropriate.The code correctly enforces a 16-byte minimum for pclmul implementations despite the PR author's observation that tests worked without it. This conservative approach is prudent until the behavior is clarified in documentation.
25109c8 to
83af008
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/crc32_test_strings_p.h (1)
17-22: Consider usinguint32_tfor CRC fields for better portability.The
crcandexpectfields useunsigned long, which varies in size across platforms (32-bit vs 64-bit). Since CRC32 values are always 32-bit, usinguint32_twould be more explicit and portable.Apply this diff:
typedef struct { - unsigned long crc; + uint32_t crc; const uint8_t *buf; size_t len; - unsigned long expect; + uint32_t expect; } crc32_test;Note: This would also require updating the test data literal suffixes (e.g.,
0x0is fine, but explicit0x0Umight be clearer) and ensuring consumers of this struct handle the type correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/test_crc32.cc
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.ccarch/generic/generic_functions.h
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.ccarch/generic/generic_functions.h
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.ccarch/generic/generic_functions.h
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-25T19:45:06.009Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:596-597
Timestamp: 2024-12-25T19:45:06.009Z
Learning: We should verify out-of-bounds pointer arithmetic in chorba_118960_nondestructive() and related loops where "input + i" is used, especially when len < expected block sizes.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-06-18T19:28:32.987Z
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.
Applied to files:
arch/generic/generic_functions.h
🧬 Code graph analysis (3)
test/CMakeLists.txt (1)
test/benchmarks/benchmark_crc32.cc (1)
crc32(21-48)
test/test_crc32_fold_copy.cc (2)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)test/test_adler32.cc (1)
adler32_variant(347-353)
arch/generic/generic_functions.h (2)
crc32.h (1)
crc32_fold_s(20-23)functable.c (1)
crc32_fold_stub(402-405)
🔇 Additional comments (4)
test/CMakeLists.txt (1)
186-186: LGTM!The addition of the new test file is correctly placed within the
ZLIBNG_ENABLE_TESTSconditional block and follows the established pattern. The comment clearly describes the purpose of the test.arch/generic/generic_functions.h (1)
15-17: LGTM!The new function pointer typedefs are well-defined and follow the existing naming conventions. The signatures correctly match the corresponding function declarations (lines 39-42) and enable clean parameterized testing of crc32_fold operations across different implementations.
test/test_crc32_fold_copy.cc (2)
21-46: LGTM!The test fixture and helper function are well-structured. The logic correctly:
- Handles the
onlyzeroconstraint to skip tests with non-zero initial CRC when needed- Enforces minimum length requirements for optimized implementations
- Verifies both CRC correctness and data copying fidelity
The use of a global buffer
dstbufat line 19 is acceptable for sequential test execution in Google Test.
72-73: The 16-byte minimum for pclmulqdq is correct and not a matter of uncertainty.The pclmulqdq instruction operates on 128-bit (m128) memory operands by definition. The memory form reads a 128-bit (m128) operand, making 16 bytes a fundamental operand requirement rather than an arbitrary constraint. The test at lines 72–73 correctly uses 16 bytes as the minimum.
83af008 to
1ee8257
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/test_crc32_fold_copy.cc (1)
19-19: Consider per-test buffer allocation for better test isolation.The global
dstbufis shared across all test instances. While GTest runs tests sequentially by default, this design could cause issues if tests ever run in parallel or if future test runners are used.Consider making
dstbufa member of the test class or allocating it per-test:+class crc32_fc_variant : public ::testing::TestWithParam<crc32_test> { +protected: + ALIGNED_(16) uint8_t dstbuf[32768]; +public:Then update line 38 to use
this->dstbufinstead of the global.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/crc32_test_strings_p.h
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
arch/generic/generic_functions.htest/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-06-18T19:28:32.987Z
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.
Applied to files:
arch/generic/generic_functions.h
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
arch/generic/generic_functions.htest/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
arch/generic/generic_functions.htest/test_crc32_fold_copy.cc
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
🧬 Code graph analysis (2)
arch/generic/generic_functions.h (2)
crc32.h (1)
crc32_fold_s(20-23)functable.c (1)
crc32_fold_stub(402-405)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🔇 Additional comments (7)
test/test_crc32.cc (2)
13-18: LGTM - Clean refactoring of test data externalization.The header reorganization properly maintains C linkage and the new
crc32_test_strings_p.hinclusion enables shared test data across test suites.
77-77: LGTM - Test instantiation updated correctly.The test instantiation now references the externalized
crc32_testsdata, maintaining consistency with the refactored test data structure.test/CMakeLists.txt (1)
186-186: LGTM - Test file properly added to build configuration.The new test file is correctly added to the test sources list with an appropriate comment describing its purpose.
arch/generic/generic_functions.h (1)
15-17: LGTM - Function typedefs properly defined.The three new typedefs for CRC32 fold operations are correctly defined and match the corresponding function signatures declared later in the file (lines 39-42). These typedefs enable the parameterized testing of different fold implementations.
test/test_crc32_fold_copy.cc (3)
21-46: LGTM - Test implementation is well-structured.The test correctly exercises the fold-copy-final sequence:
- Handles optional parameters (onlyzero, minlen requirements)
- Verifies both CRC correctness and data copying fidelity
- Includes proper NULL checks before buffer operations
48-57: LGTM - Test macro and instantiation follow established patterns.The parameterized test setup correctly uses the shared
crc32_testsdata and theTEST_CRC32_FOLDmacro properly handles feature detection and test skipping for unsupported architectures.
60-82: LGTM - Architecture-specific tests properly configured.The tests correctly implement:
- Generic baseline test accepting any length
- Native and optimized variants with appropriate minimum length requirements
- Proper feature detection guards for each architecture
- The 16-byte minimum for pclmul functions is appropriately documented as uncertain pending clarification (as noted in the PR objectives)
1ee8257 to
654dc7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/test_crc32_fold_copy.cc (2)
22-47: Guard dstbuf size against test vector length.
dstbufis fixed at 32 KiB, butfold_copyandmemcmpoperate onparams.lenwithout checking that it fits the buffer. If anycrc32_testsentry exceeds 32768 bytes, this will overflow in the test.I’d recommend either asserting the assumption or adjusting the buffer size, e.g.:
ASSERT_LE(params.len, sizeof(dstbuf));just before calling
fold_copy. This keeps the “no heap allocs” requirement while making the size contract explicit and future-proof. Based on learnings.
52-82: Double-check per-variantminlen/onlyzeroconstraints and reset/final pairing.The macro wiring looks consistent (generic + native/arch variants, gated by
test_cpu_features), but given the subtle differences between implementations:
onlyzero = 1andminlen = 16for native/pclmul/vpclmulonlyzero = 0andminlen = 0for generic/ARM/LoongArch, usingcrc32_fold_reset_c/crc32_fold_final_cplus arch-specific copy functionsit’s worth re-confirming that these flags match the actual guarantees of each implementation (esp. pclmul/vpclmul minimum-length and initial-CRC behavior), and that the arch-specific
*_fold_copyfunctions are compatible with the c reset/final semantics.No change strictly required if you’ve already validated this, but a quick cross-check against the implementation comments would prevent subtle test blind spots.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/CMakeLists.txt
- arch/generic/generic_functions.h
- test/crc32_test_strings_p.h
- test/test_crc32.cc
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/test_crc32_fold_copy.cc
🧬 Code graph analysis (1)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🔇 Additional comments (1)
test/test_crc32_fold_copy.cc (1)
20-50: Good reuse of parameterized CRC test data and shared helper.The
crc32_fc_variantfixture plusTEST_CRC32_FOLDmacro cleanly centralize the fold-copy checks across all implementations, mirroring the existing crc32 test suite and keeping things easy to extend for new variants. Thesupport_flag+GTEST_SKIP()pattern also keeps unsupported paths tidy.Looks solid.
Also applies to: 61-71
654dc7b to
141cf50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/test_crc32_fold_copy.cc (1)
21-24: Whitespace/style nit: add blank line beforepublic:(CodeFactor)Minor style issue only: CodeFactor flags that
public:should be preceded by a blank line. You can appease it by inserting an empty line between theprotectedblock andpublic:.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/CMakeLists.txt
- arch/generic/generic_functions.h
- test/crc32_test_strings_p.h
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/test_crc32.cctest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-25T19:45:06.009Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:596-597
Timestamp: 2024-12-25T19:45:06.009Z
Learning: We should verify out-of-bounds pointer arithmetic in chorba_118960_nondestructive() and related loops where "input + i" is used, especially when len < expected block sizes.
Applied to files:
test/test_crc32_fold_copy.cc
🧬 Code graph analysis (2)
test/test_crc32.cc (1)
crc32.c (1)
crc32(39-41)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🪛 GitHub Check: CodeFactor
test/test_crc32_fold_copy.cc
[notice] 24-24: test/test_crc32_fold_copy.cc#L24
"public:" should be preceded by a blank line. (whitespace/blank_line)
🔇 Additional comments (2)
test/test_crc32.cc (1)
13-19: Shared crc32_tests header usage and instantiation look goodIncluding
crc32_test_strings_p.hunderextern "C"and switching the instantiation tocrc32_testskeeps the existing test behavior while de-duplicating vectors for reuse in the fold-copy tests. No issues spotted with linkage or parametrization.Also applies to: 77-77
test/test_crc32_fold_copy.cc (1)
52-82: Macro wiring and feature guards for fold-copy variants look correctThe
TEST_CRC32_FOLDmacro usage matches thecrc32_fold_*typedefs, and the feature flags (ARM, PCLMULQDQ, VPCLMULQDQ, LOONGARCH, native) mirror the existingtest_crc32.ccpattern. Once the skip-condition bug is fixed, this setup should give good coverage across the different implementations.
141cf50 to
3557464
Compare
3557464 to
1aaf230
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/test_crc32_fold_copy.cc (2)
32-32: Minor style: add space afterif.Apply this diff:
- if(params.buf == NULL || params.len < minlen || (onlyzero && params.crc != 0)) { + if (params.buf == NULL || params.len < minlen || (onlyzero && params.crc != 0)) {
71-73: Document the 16-byte minimum rationale.The comment questions whether 16 bytes is truly the minimum for pclmul functions. Per the PR objectives, you observed these instructions working with no minimum in testing but retained the 16-byte requirement pending clarification (possibly due to implementations reading beyond the buffer).
Consider updating the comment to reflect this finding and the reason for the conservative choice, or verify the actual requirement with the implementation maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- arch/generic/generic_functions.h
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cctest/test_crc32.cc
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cctest/test_crc32.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cctest/test_crc32.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cctest/test_crc32.cc
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
🧬 Code graph analysis (2)
test/crc32_test_strings_p.h (1)
test/benchmarks/benchmark_crc32.cc (1)
crc32(21-48)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🔇 Additional comments (5)
test/CMakeLists.txt (1)
186-186: LGTM!The test file is properly integrated into the build system under the
ZLIBNG_ENABLE_TESTSguard, consistent with other internal test additions.test/crc32_test_strings_p.h (1)
1-183: Excellent test data externalization.The header properly externalizes CRC32 test vectors for reuse across multiple test files, with comprehensive coverage including edge cases (null pointers, zero-length, various sizes up to 32KB). Include guards are correctly implemented.
test/test_crc32.cc (1)
8-16: Clean refactoring to use external test data.The transition from local test data to the external
crc32_testsarray eliminates duplication and enables test data reuse across multiple test files.Also applies to: 72-72
test/test_crc32_fold_copy.cc (2)
23-44: Well-structured test implementation.The test properly validates both CRC correctness and data copy fidelity, with appropriate safety checks and skip logic for unsupported test cases. The approach correctly handles implementations that have minimum length requirements or only support zero initial CRC values.
59-79: Comprehensive test coverage across implementations.The test instantiations properly cover the generic C implementation and all architecture-specific optimizations (ARM, x86 pclmul/vpclmul, LoongArch), with appropriate feature flags and implementation-specific constraints (
minlen,onlyzero).
1aaf230 to
f88fe56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/crc32_test_strings_p.h (2)
4-15: Consider moving static data to a source file.Defining static data in a header causes each translation unit that includes it to get its own copy of the buffer and test setup function, leading to code bloat. Additionally,
buf32kis initialized via a function call at static initialization time in each TU.For better maintainability and to avoid duplication, consider moving these definitions to a
.cfile and exposing them viaexterndeclarations in the header.Optional: Use sized type for loop variable.
The loop variable at line 7 uses
int, which could be replaced withsize_tor an explicit sized type for consistency with modern C practices.Apply this diff if adopting the loop variable change:
- for (int i = 0; i < 32768; ++i) { + for (size_t i = 0; i < 32768; ++i) {
17-22: Optional: Consider using fixed-width types for CRC values.The struct uses
unsigned longfor CRC values, which varies in size across platforms (32-bit on some, 64-bit on others). Modern code typically usesuint32_tfor CRC-32 values for explicit size guarantees and portability.However, this may be intentional for compatibility with zlib's API, which uses
unsigned long. If cross-platform consistency is important, consider usinguint32_tinstead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/CMakeLists.txt
- test/test_crc32_fold_copy.cc
- arch/generic/generic_functions.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32.cc
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/crc32_test_strings_p.h
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32.cc
🧬 Code graph analysis (1)
test/crc32_test_strings_p.h (1)
test/benchmarks/benchmark_crc32.cc (1)
crc32(21-48)
🔇 Additional comments (3)
test/test_crc32.cc (2)
8-16: LGTM: Clean refactoring to use external test data.The includes are properly organized with C headers wrapped in
extern "C"for correct linkage. The new test data header is correctly included for reuse across test files.
72-72: LGTM: Test instantiation correctly updated.The parameterized test suite now references
crc32_testsfrom the external header, enabling test case reuse across multiple test files.test/crc32_test_strings_p.h (1)
25-181: LGTM: Comprehensive test coverage.The test array provides excellent coverage with:
- NULL pointer and zero-length edge cases
- Small strings and various lengths
- Initial CRC values (both 0x0 and non-zero)
- Large buffer tests using
buf32k- Multiple character sets and patterns
The test data will be valuable for validating CRC implementations across architectures.
f88fe56 to
4024d53
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/crc32_test_strings_p.h (1)
17-22: Consider usinguint32_tfor CRC fields.The struct uses
unsigned longforcrcandexpectfields (lines 18, 21), but CRC-32 values are always 32-bit. Usinguint32_twould make the intent more explicit and avoid potential portability concerns on platforms whereunsigned longis 64-bit.Apply this diff if you'd like to make the types more explicit:
typedef struct { - unsigned long crc; + uint32_t crc; const uint8_t *buf; size_t len; - unsigned long expect; + uint32_t expect; } crc32_test;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test_crc32.cc
- test/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/crc32_test_strings_p.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.htest/test_crc32_fold_copy.cc
📚 Learning: 2025-06-18T19:28:32.987Z
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.
Applied to files:
arch/generic/generic_functions.h
📚 Learning: 2025-01-06T07:20:02.028Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:0-0
Timestamp: 2025-01-06T07:20:02.028Z
Learning: Business requirement: Additional memory allocations should be avoided, stack allocations might be used instead.
Applied to files:
test/test_crc32_fold_copy.cc
📚 Learning: 2024-12-27T10:06:16.184Z
Learnt from: samrussell
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_braid_c.c:215-217
Timestamp: 2024-12-27T10:06:16.184Z
Learning: There's a business requirement to use stack allocation instead of heap for the Chorba buffer.
Applied to files:
test/test_crc32_fold_copy.cc
🧬 Code graph analysis (2)
arch/generic/generic_functions.h (1)
crc32.h (1)
crc32_fold_s(20-23)
test/test_crc32_fold_copy.cc (1)
arch/arm/crc32_armv8.c (1)
crc32_fold_copy_armv8(76-79)
🔇 Additional comments (4)
arch/generic/generic_functions.h (1)
15-17: LGTM! Well-structured function pointer typedefs.The three new typedefs for crc32_fold operations are clean, consistent with existing patterns in the file, and properly align with the actual function declarations on lines 39-42. This enables parameterized testing across multiple implementations.
test/crc32_test_strings_p.h (1)
1-183: LGTM! Test data extraction is well-structured.The header successfully consolidates CRC test vectors for reuse across test files. The include guards, alignment, and test case coverage are all appropriate. The static storage pattern (each translation unit gets its own copy) is acceptable for a test-only header, though it does introduce some duplication.
test/test_crc32_fold_copy.cc (2)
17-45: LGTM! Well-structured test class with proper validation.The test implementation correctly:
- Bounds-checks the input length against the buffer size (line 28)
- Skips unsupported test cases (NULL buffer, insufficient length, or non-zero initial CRC when required to be zero) on lines 32-34
- Validates both the CRC result and the copied data (lines 42-43)
The 33KB member buffer is large but acceptable for tests, and moving it to a member variable (vs. local) avoids stack allocation ordering issues.
71-72: The 16-byte requirement for PCLMULQDQ is due to Intel instruction specifications, not implementation tolerances.The PCLMULQDQ instruction requires m128 (16-byte) memory operand per Intel SDM specification. The implementation in
arch/x86/crc32_fold_pclmulqdq_tpl.hhandles this correctly: it contains an assertionassert(len >= 16 || first == 0)with a comment explaining that callers enforce the 16-byte minimum. For buffers smaller than 16 bytes, the code defensively copies the input into a 16-byte-aligned partial buffer before performing the load, rather than relying on out-of-bounds reads. The test minimum of 16 bytes reflects the instruction's inherent requirement and the API contract enforced by callers.
4024d53 to
dc26119
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/crc32_test_strings_p.h (2)
1-3: Consider adding necessary includes.The header uses
uint8_t,size_t, and theALIGNED_macro without including their definitions. While this works if consuming files include the right headers first, it creates a fragile dependency on include order.Consider adding at the top (after the include guard):
#ifndef CRC32_TEST_STRINGS_P_H #define CRC32_TEST_STRINGS_P_H +#include <stddef.h> /* size_t */ +#include <stdint.h> /* uint8_t */ +#include "zutil.h" /* ALIGNED_ */ + ALIGNED_(16) static uint8_t crc32_fullwin_buf[32768];
6-13: Consider usingsize_tfor the loop variable.The loop variable is declared as
intwhile iterating over a buffer ofsize_tlength. Usingsize_twould be more consistent and idiomatic.static uint8_t* crc32_setup_buf() { - for (int i = 0; i < 32768; ++i) { + for (size_t i = 0; i < 32768; ++i) { uint8_t ic = (uint8_t)(i % 256); crc32_fullwin_buf[i] = ic; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
arch/generic/generic_functions.h(1 hunks)test/CMakeLists.txt(1 hunks)test/crc32_test_strings_p.h(1 hunks)test/test_crc32.cc(2 hunks)test/test_crc32_fold_copy.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- test/test_crc32.cc
- test/test_crc32_fold_copy.cc
- test/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
📚 Learning: 2025-01-23T22:01:53.422Z
Learnt from: Dead2
Repo: zlib-ng/zlib-ng PR: 1837
File: arch/generic/crc32_c.c:19-29
Timestamp: 2025-01-23T22:01:53.422Z
Learning: The Chorba CRC32 functions (crc32_chorba_118960_nondestructive, crc32_chorba_32768_nondestructive, crc32_chorba_small_nondestructive, crc32_chorba_small_nondestructive_32bit) are declared in crc32_c.h.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.h
📚 Learning: 2025-02-21T01:42:40.488Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-21T01:42:40.488Z
Learning: In the SSE2-optimized Chorba CRC implementation (chorba_small_nondestructive_sse), the input buffer length is enforced to be a multiple of 16 bytes due to SSE2 operations, making additional checks for smaller alignments (like 8 bytes) redundant.
Applied to files:
test/crc32_test_strings_p.h
📚 Learning: 2024-12-22T20:40:03.280Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1833
File: test/test_crc32.cc:0-0
Timestamp: 2024-12-22T20:40:03.280Z
Learning: In the crc32_align test class (test/test_crc32.cc), the alignment offset parameter is always guaranteed to be non-negative because the test parameters are controlled and it never makes sense to have a negative offset.
Applied to files:
test/crc32_test_strings_p.h
📚 Learning: 2025-02-21T01:41:50.358Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:14-24
Timestamp: 2025-02-21T01:41:50.358Z
Learning: In zlib-ng's SSE2 vectorized Chorba CRC implementation, the code that calls READ_NEXT macro ensures 16-byte alignment, making explicit alignment checks unnecessary within the macro.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.h
📚 Learning: 2025-02-23T16:49:52.043Z
Learnt from: KungFuJesus
Repo: zlib-ng/zlib-ng PR: 1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-23T16:49:52.043Z
Learning: In zlib-ng, bounds checking for CRC32 computation is handled by the caller, not within the individual CRC32 implementation functions like `crc32_chorba_sse2`.
Applied to files:
test/crc32_test_strings_p.harch/generic/generic_functions.h
📚 Learning: 2025-06-18T19:28:32.987Z
Learnt from: phprus
Repo: zlib-ng/zlib-ng PR: 1925
File: arch/loongarch/slide_hash_lsx.c:64-64
Timestamp: 2025-06-18T19:28:32.987Z
Learning: In zlib-ng's slide_hash functions, when calling slide_hash_chain(), the fourth parameter should be the window size (wsize) representing the number of entries in the prev table, not s->w_size directly. The local variable wsize is typically cast to the appropriate type (uint16_t) from s->w_size for this purpose.
Applied to files:
arch/generic/generic_functions.h
🔇 Additional comments (2)
arch/generic/generic_functions.h (1)
15-17: LGTM! Function pointer typedefs correctly defined.The three new typedefs for crc32_fold operations are correctly aligned with their corresponding function declarations (lines 39-42) and follow the existing naming conventions. These additions appropriately extend the API surface to support the new test infrastructure.
test/crc32_test_strings_p.h (1)
25-181: LGTM! Comprehensive test coverage.The test cases provide excellent coverage:
- NULL pointer handling (lines 26-33) correctly expects 0 return per the API contract
- Zero-length inputs (lines 34-35)
- Various string inputs with known CRC values (lines 36-104)
- Incremental CRC computation with non-zero initial values (lines 105-171)
- Large inputs including 600-byte and 32KB buffers (lines 172-180)
What I wanted was to add benchmarks for crc32_fold_copy, what I ended up implementing was tests.
I reused the tests from the crc32 tests, to make it easier to add new ones.
This passes testing on x86_64 fine.
Added tests for ARMv8 and Loongarch as well, even though those actually just do the crc followed by a memcpy instead of doing the copy during the actual checksumming.
For some reason the [v]pclmul claim 16 bytes is minimum, but in my tests they work with no minimum as well. Not sure whether I was just lucky or that 16byte minimum is not actually a minimum. Perhaps it does read beyond the end of the buffer but as long as that is allowed, it is safe? IDK, so I kept it at 16 for now.