Conversation
WalkthroughCentralizes the bit-reversal helper by introducing a static inline bi_reverse in deflate_p.h, updates callers to use bi_reverse directly, removes the old prototype from deflate.h, adds needed includes, and deletes the local implementation in trees.c. The s390 dfltcc path and maketrees tool are updated to include deflate_p.h and call bi_reverse. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
deflate_p.h (2)
100-111: Prevent macro leakage from header by scoping bitrev8 locallyDefining bitrev8 as a preprocessor macro inside a widely-included header leaks it to the entire translation unit. Even though redefinition with identical bodies is allowed, it’s easy to collide with another symbol or alter compile diagnostics. Prefer a header-local name and immediately undef it after use.
Apply this minimal, low-risk change to confine the helper:
static inline uint16_t bi_reverse(unsigned code, int len) { /* code: the value to invert */ /* len: its bit length */ Assert(len >= 1 && len <= 15, "code length must be 1-15"); -#define bitrev8(b) \ - (uint8_t)((((uint8_t)(b) * 0x80200802ULL) & 0x0884422110ULL) * 0x0101010101ULL >> 32) - return (bitrev8(code >> 8) | (uint16_t)bitrev8(code) << 8) >> (16 - len); +#define ZNG_BITREV8(b) \ + (uint8_t)((((uint8_t)(b) * 0x80200802ULL) & 0x0884422110ULL) * 0x0101010101ULL >> 32) + uint16_t r = (ZNG_BITREV8(code >> 8) | (uint16_t)ZNG_BITREV8(code) << 8) >> (16 - len); +#undef ZNG_BITREV8 + return r; }
100-111: Optional: leverage compiler bit-reversal builtins when availableSome compilers provide __builtin_bitreverse16, which can be faster and clearer. Keep the current path as a portable fallback.
Example (outside this hunk, for illustration):
/* Optional fast path */ #if defined(__has_builtin) # if __has_builtin(__builtin_bitreverse16) return (uint16_t)__builtin_bitreverse16((uint16_t)code) >> (16 - len); # endif #endif /* Fallback: existing implementation */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
arch/s390/dfltcc_deflate.c(2 hunks)deflate.h(0 hunks)deflate_p.h(1 hunks)tools/maketrees.c(2 hunks)trees.c(2 hunks)
💤 Files with no reviewable changes (1)
- deflate.h
🧰 Additional context used
🧬 Code graph analysis (3)
tools/maketrees.c (1)
deflate_p.h (1)
bi_reverse(103-110)
arch/s390/dfltcc_deflate.c (1)
deflate_p.h (1)
bi_reverse(103-110)
trees.c (1)
deflate_p.h (1)
bi_reverse(103-110)
🔇 Additional comments (6)
tools/maketrees.c (2)
9-9: Including deflate_p.h here is reasonableBringing in the inline bi_reverse via deflate_p.h for the generator keeps a single source of truth. No concerns.
94-95: Direct call to inlined bi_reverse matches original semanticsUsing bi_reverse((unsigned)n, 5) is correct for the static distance codes (5-bit fixed). Good change.
arch/s390/dfltcc_deflate.c (2)
18-18: Header inclusion aligns this TU with the new inline helperAdding deflate_p.h is needed now that bi_reverse lives there. Looks good.
94-95: Validateparam->eoblRangeI didn’t find any code path that initializes or checks the 4-bit
eoblfield before it’s passed tobi_reverse(). Sinceuint8_t eobl:4can legally be 0–15, we need to ensure the hardware DFLTCC deflate always returns a non-zero length (1–15). If that guarantee isn’t documented in the hardware spec, add a defensive check or assertion immediately before the call to prevent a zero-length shift:// arch/s390/dfltcc_deflate.c: around line 94 Z_ASSERT(param->eobl >= 1 && param->eobl <= 15); send_bits(state, bi_reverse(param->eobs >> (15 - param->eobl), param->eobl), param->eobl, state->bi_buf, state->bi_valid);• Declaration: arch/s390/dfltcc_common.h (
uint8_t eobl : 4;)
• Usage: arch/s390/dfltcc_deflate.c (lines 94–95)trees.c (2)
35-35: Including deflate_p.h here is appropriateThis replaces the former local implementation and keeps the helper centralized. No issues.
403-404: bi_reverse usage remains correct for Huffman code generationReplacing PREFIX(bi_reverse) with the inlined bi_reverse preserves behavior. The code index increment (next_code[len]++) is unchanged and correct.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1955 +/- ##
===========================================
+ Coverage 81.47% 81.96% +0.49%
===========================================
Files 162 162
Lines 13956 13960 +4
Branches 3120 3127 +7
===========================================
+ Hits 11371 11443 +72
+ Misses 1594 1553 -41
+ Partials 991 964 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Brings back memories, ... #948 |
It surprised me to find that this was not already being inlines, seeing how small and simple the function is.
Inlining it reduces the size of the library by ~160 bytes on x86-64.
Performance difference is very minimal, can't really tell for certain from the benchmarks, but logic says it should be a bit faster.
Develop
PR
Summary by CodeRabbit
Refactor
Chores
Notes