refactor: php_brotli_context struct #60
Conversation
WalkthroughThis change refactors the internal management of Brotli compression contexts in the PHP extension. It introduces a new struct, Changes
Sequence Diagram(s)sequenceDiagram
participant PHP_User
participant BrotliExtension
participant BrotliContext
PHP_User->>BrotliExtension: Request compress/decompress
BrotliExtension->>BrotliContext: php_brotli_context_init()
BrotliExtension->>BrotliContext: Use encoder/decoder, buffers
BrotliExtension->>BrotliContext: php_brotli_context_close()
BrotliContext-->>BrotliExtension: Resources cleaned up
BrotliExtension-->>PHP_User: Return result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
brotli.c (3)
145-154: Minor nit – usememsetto zero the struct
php_brotli_context_init()manually assigns eight members. A singlememset(ctx, 0, sizeof(*ctx));(orZEND_SECURE_ZERO) would reduce maintenance overhead when new fields are added.-static void php_brotli_context_init(php_brotli_context *ctx) -{ - ctx->encoder = NULL; - ctx->decoder = NULL; - ctx->available_in = 0; - ctx->next_in = NULL; - ctx->available_out = 0; - ctx->next_out = NULL; - ctx->output = NULL; -} +static void php_brotli_context_init(php_brotli_context *ctx) +{ + memset(ctx, 0, sizeof(*ctx)); +}
1250-1256: Type-safety nit – cast input buffer toconst uint8_t*
ctx->next_inis declaredconst uint8_t *, butin_bufis achar *.
Assigning without a cast is safe but produces an implicit-conversion warning on some compilers.- ctx->next_in = in_buf; + ctx->next_in = (const uint8_t *)in_buf;
1263-1274: Resource leak after FINISH?After a
BROTLI_OPERATION_FINISH, the encoder remains allocated until the context object is destroyed.
If user code keeps the object around but never reuses it, memory stays occupied unnecessarily. Consider automatically callingphp_brotli_context_close()(or at least destroyingctx->encoder) when FINISH succeeds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
brotli.c(14 hunks)php_brotli.h(1 hunks)
🔇 Additional comments (3)
php_brotli.h (1)
31-31:✅ Verification successful
Opaque‐struct change looks good, but double-check for out-of-file field access
Forward-declaring
_php_brotli_contexttightens encapsulation – great move.
Please verify that no other compilation units still dereference members such asctx->encoder/ctx->decoderdirectly; otherwise they will now fail to compile.
🏁 Script executed:
#!/bin/bash # Find any use of php_brotli_context members outside brotli.c rg --multiline --line-number $'php_brotli_context\\s*\\*[^;]*\\->' | grep -v 'brotli.c'Length of output: 83
Let's inspect the struct’s members and look specifically for any
encoder/decoderaccesses outsidebrotli.c:
🏁 Script executed:
#!/bin/bash # Show struct fields in php_brotli.h rg --line-number 'struct _php_brotli_context' -A 10 php_brotli.h # Search for encoder/decoder member access outside brotli.c rg --line-number '->encoder' | grep -v 'brotli.c' rg --line-number '->decoder' | grep -v 'brotli.c'Length of output: 1334
Let’s rerun the member‐access search using fixed string matching and explicitly excluding the implementation file:
🏁 Script executed:
#!/bin/bash # Search for any ctx->encoder or ctx->decoder outside brotli.c rg -n -F '->encoder' . | grep -v 'brotli.c' rg -n -F '->decoder' . | grep -v 'brotli.c'Length of output: 897
Let’s retry with a plain
grepsearch—which handles leading “->” cleanly—and excludebrotli.cexplicitly:
🏁 Script executed:
#!/bin/bash # Check for any ctx->encoder or ctx->decoder uses outside brotli.c grep -R "->encoder" -n . | grep -v "brotli.c:" grep -R "->decoder" -n . | grep -v "brotli.c:"Length of output: 315
Let’s retry with
grep --to avoid option parsing on the->:
🏁 Script executed:
#!/bin/bash # Look for any ctx->encoder or ctx->decoder outside brotli.c grep -R -n -- 'ctx->encoder' . | grep -v 'brotli\.c:' grep -R -n -- 'ctx->decoder' . | grep -v 'brotli\.c:'Length of output: 111
No external
php_brotli_contextfield accesses detected—change approved.A search for any
ctx->encoderorctx->decoderreferences outsidebrotli.creturned no results, confirming the opaque‐struct encapsulation introduces no breakage.brotli.c (2)
124-133: Struct refactor LGTMThe consolidated
_php_brotli_contextclearly separates encoder/decoder state and keepszend_object stdas the last member, so existingXtOffsetOf(..., std)math remains valid.
No functional issues spotted.
156-176: Centralised cleanup handles partial initialisation
php_brotli_context_close()correctly guards each resource and NULLs pointers, making the function idempotent – nice work.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary by CodeRabbit