deflate_stored.c (2)

97-101: Use the returned byte count from read_buf to update out pointers

read_buf returns how many bytes were actually copied. While len is expected to be available here, using the return value makes the code robust and self-documenting.

Apply this diff:

-            read_buf(s->strm, s->strm->next_out, len);
-            s->strm->next_out += len;
-            s->strm->avail_out -= len;
-            s->strm->total_out += len;
+            unsigned got = read_buf(s->strm, s->strm->next_out, len);
+            s->strm->next_out  += got;
+            s->strm->avail_out -= got;
+            s->strm->total_out += got;

---

160-163: Likewise, prefer read_buf’s return value when updating window/write indices

As above, using the actual number of bytes read avoids any future fragility if the invariants change.

Apply this diff:

-        read_buf(s->strm, s->window + s->strstart, have);
-        s->strstart += have;
-        s->insert += MIN(have, s->w_size - s->insert);
+        unsigned got = read_buf(s->strm, s->window + s->strstart, have);
+        s->strstart += got;
+        s->insert += MIN(got, s->w_size - s->insert);

deflate.c (2)

781-792: Direct use of flush_pending_inline in hot path is fine; consider consistency

Using the inline in the tight loop removes call overhead. Elsewhere in this function we still call PREFIX(flush_pending). It’s okay to mix, but consider standardizing call sites for consistency (either all inline in hot paths, wrapper elsewhere), to reduce reader surprise.

---

835-841: Minor: consider using flush_pending_inline here too

Not performance-critical, but for symmetry with other header/trailer flushes you could call flush_pending_inline here as well.

Apply this diff if you prefer consistency:

-        PREFIX(flush_pending)(strm);
+        flush_pending_inline(strm);

deflate_p.h (1)

133-156: read_buf inline mirrors previous semantics; checksum handling looks solid

• Returns 0 early if no input; proper min(size, avail_in).
• Handles checksum updates:
  • wrap == 2: crc32_fold_copy (guarded by GZIP)
  • wrap == 1: adler32_fold_copy
  • else/no checksum: memcpy
• Updates avail_in, next_in, total_in.

One tiny nit: if you expect any future call sites to rely on overlapping next_in/target buffers (unlikely for deflate), memmove would be safer than memcpy in the no-checksum branches. Current API assumes non-overlapping buffers, so leaving memcpy is fine.

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.

Learnt from: KungFuJesus
PR: zlib-ng/zlib-ng#1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-18T02:53:23.621Z
Learning: In zlib-ng, when macro names and parameters are self-explanatory and scalar versions of the code provide context, additional documentation for the macros may not be necessary.

deflate_p.h (4)

101-126: flush_pending_inline: Semantics look correct; consider unifying with out-of-line wrapper to avoid divergence

The implementation mirrors the classic flush_pending: flush bits, copy min(pending, avail_out), bump pointers/counters, and reset pending_out when empty. Looks good.

Optional: to minimize the risk of the inline and the out-of-line PREFIX(flush_pending) drifting over time, make the latter a thin wrapper around this inline helper (or have FLUSH_BLOCK call the inline). This keeps one source of truth while still enabling inlining where used.

As a follow-up, consider changing the non-inlined wrapper in deflate.c to:

/* deflate.c */
Z_INTERNAL void PREFIX(flush_pending)(PREFIX3(stream) *strm) {
    flush_pending_inline(strm);
}

And/or update the macro here to use the inline helper:

/* deflate_p.h */
#define FLUSH_BLOCK_ONLY(s, last) { \
    zng_tr_flush_block(s, (s->block_start >= 0 ? \
                   (char *)&s->window[(unsigned)s->block_start] : \
                   NULL), \
                   (uint32_t)((int)s->strstart - s->block_start), \
                   (last)); \
    s->block_start = (int)s->strstart; \
    flush_pending_inline(s->strm); \
}

---

101-126: Minor type consistency nit: consider uInt for len

len derives from MIN(s->pending, strm->avail_out). Since avail_out is uInt, using uInt for len avoids implicit conversions when updating avail_out, while keeping the same range characteristics on supported platforms.

-static inline void flush_pending_inline(PREFIX3(stream) *strm) {
-    uint32_t len;
+static inline void flush_pending_inline(PREFIX3(stream) *strm) {
+    uInt len;

---

133-155: Micro-optimization: avoid repeated strm->state dereferences

Hoisting strm->state to a local helps codegen and readability, and shortens the FUNCTABLE_CALL argument for crc/adler paths.

 static inline unsigned read_buf(PREFIX3(stream) *strm, unsigned char *buf, unsigned size) {
-    uint32_t len = MIN(strm->avail_in, size);
+    uint32_t len = MIN(strm->avail_in, size);
+    deflate_state *s = strm->state;
@@
-    if (!DEFLATE_NEED_CHECKSUM(strm)) {
+    if (!DEFLATE_NEED_CHECKSUM(strm)) {
         memcpy(buf, strm->next_in, len);
 #ifdef GZIP
-    } else if (strm->state->wrap == 2) {
-        FUNCTABLE_CALL(crc32_fold_copy)(&strm->state->crc_fold, buf, strm->next_in, len);
+    } else if (s->wrap == 2) {
+        FUNCTABLE_CALL(crc32_fold_copy)(&s->crc_fold, buf, strm->next_in, len);
 #endif
-    } else if (strm->state->wrap == 1) {
+    } else if (s->wrap == 1) {
         strm->adler = FUNCTABLE_CALL(adler32_fold_copy)(strm->adler, buf, strm->next_in, len);
     } else {
         memcpy(buf, strm->next_in, len);
     }

---

133-155: Consider force-inlining for hot paths

If the intent is to realize consistent perf wins across compilers/optimization levels, using the project’s force-inline attribute macro (if available, e.g., Z_FORCEINLINE) can help ensure these helpers are inlined, especially in non-LTO builds.

-static inline void flush_pending_inline(PREFIX3(stream) *strm) {
+Z_FORCEINLINE static void flush_pending_inline(PREFIX3(stream) *strm) {
@@
-static inline unsigned read_buf(PREFIX3(stream) *strm, unsigned char *buf, unsigned size) {
+Z_FORCEINLINE static unsigned read_buf(PREFIX3(stream) *strm, unsigned char *buf, unsigned size) {

If Z_FORCEINLINE isn’t available or desired, ignore this.

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.

deflate.c (1)

51-54: Dup include of functable.h (benign); consider relying on deflate_p.h

deflate_p.h already includes functable.h. Keeping both is harmless thanks to include guards, but you can remove this one to reduce redundancy.

Apply:

-#include "functable.h"

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.

Learnt from: KungFuJesus
PR: zlib-ng/zlib-ng#1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-18T02:53:23.621Z
Learning: In zlib-ng, when macro names and parameters are self-explanatory and scalar versions of the code provide context, additional documentation for the macros may not be necessary.

arch/s390/dfltcc_deflate.c (1)

93-105: Behavior preserved: send_eobs() still flushes bits then pending bytes

Switching to flush_pending_inline(strm) correctly flushes bit buffer first (via zng_tr_flush_bits inside the inline) and copies out pending bytes. The subsequent memmove-on-remaining-pending logic remains valid, since pending_out is only reset to pending_buf when pending == 0. LGTM.

deflate_p.h (2)

12-13: functable.h inclusion is appropriate here

Required for FUNCTABLE_CALL used below and keeps inline helpers self-contained. LGTM.

---

101-126: Inline flush is correct and side-effect complete

flush_pending_inline() flushes pending bits first, respects avail_out, updates all counters, and restores pending_out when drained. Matches legacy behavior while enabling inlining on hot paths.

deflate.c (5)

743-748: Wrapper retained for ABI/internal call sites; implementation delegates to inline

The wrapper now just calls flush_pending_inline, preserving symbol presence while enabling selective inlining elsewhere. LGTM.

---

781-791: Hot path switch to inline flush is appropriate

Inlining the flush on the main deflate() path is consistent with the PR goal and should help a bit with register allocation and call overhead in tight loops.

---

966-972: Header/HCRC path uses inline flush — fine

Using flush_pending_inline here ensures the buffer is empty before proceeding, matching prior behavior while allowing the compiler to inline when profitable.

---

1043-1054: Trailer path uses inline flush — fine

Keeps semantics identical (trailer written once, then flush). LGTM.

---

1226-1227: Switched fill_window() to new read_buf helper

The inline read_buf updates checksum via fold-copy when needed and advances stream counters atomically. This centralizes I/O handling and is a good change.

deflate_p.h (1)

101-126: flush_pending_inline: correct semantics; tiny branch-hint nit

Logic matches the previous non-inlined helper: flush bit buffer, copy min(pending, avail_out), update pointers/counters, and reset pending_out when empty. Consider adding an unlikely-branch hint for the early return to help the predictor in hot paths.

-    if (len == 0)
+    if (UNLIKELY(len == 0))
         return;

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.

Learnt from: KungFuJesus
PR: zlib-ng/zlib-ng#1872
File: arch/x86/chorba_sse2.c:0-0
Timestamp: 2025-02-18T02:53:23.621Z
Learning: In zlib-ng, when macro names and parameters are self-explanatory and scalar versions of the code provide context, additional documentation for the macros may not be necessary.

deflate_p.h (2)

12-13: LGTM: functable.h include is appropriate for FUNCTABLE_CALL usage

Including functable.h here is correct since read_buf and (gzip/adler) fold copies rely on FUNCTABLE_CALL. Include guards prevent double inclusion with deflate.c.

---

127-156: No stale PREFIX(read_buf) calls—read_buf is correctly inlined

A ripgrep search confirmed that there are no call sites using the removed PREFIX(read_buf) macro, and existing calls in deflate_stored.c and deflate.c invoke the bare read_buf, which is now provided as a static inline in deflate_p.h:

• deflate_stored.c: calls to read_buf(s->strm, …) at lines 97 and 160
• deflate.c: call to n = read_buf(…) at line 1226
• No hits for PREFIX(read_buf)(…) outside deflate_p.h

Since each translation unit including deflate_p.h will see the inline definition of read_buf, there are no unresolved symbols to worry about. This comment can be resolved as no further changes are needed.

deflate.c (6)

51-51: Include order is fine

functable.h is needed here because this TU directly uses FUNCTABLE_CALL (e.g., adler32/crc32/slide_hash). Keeping it above deflate.h is consistent with prior style.

---

743-749: Wrapper preserves ABI while enabling hot-path inlining

Keeping Z_INTERNAL PREFIX(flush_pending) as a thin wrapper over flush_pending_inline retains the exported symbol and avoids code bloat where the wrapper is used in colder paths.

---

779-792: Good: hot-path switch to flush_pending_inline

Inlining here avoids a function call in the common pending != 0 case and pairs well with the earlier bit-buffer flush in the inline. No functional issues spotted.

---

966-972: Header/HCRC transition: inline flush is appropriate

Using flush_pending_inline here keeps the pipeline warm as we transition states. Matches the partial inlining strategy noted in the PR description.

---

1043-1044: Trailer flush: inline usage is consistent

Inlining at the trailer write is sensible, given it’s on the finishing hot path. State updates remain identical to the wrapper.

---

1226-1227: read_buf callsites fully migrated and verified

All calls to read_buf now resolve to the new TU-local inline defined in deflate_p.h. No legacy wrapper calls remain.

• deflate.c (lines 1226–1227): calls inline read_buf as intended.
• deflate_stored.c (lines 97 and 160): also invoke the inline version from deflate_p.h.

No further changes required.