Conversation
Fixes scope issues in `@buffer`
There was a problem hiding this comment.
Pull request overview
This PR updates the @buffer macros to avoid the let/try...finally structure that introduced scope issues, and records the change in the changelog for the v0.2.2 release.
Changes:
- Modify the single-argument
@buffer(specs, ex)macro to allocate theMAllocBufferdirectly in the caller scope, runex, then free and clear the buffer variable without usingletortry...finally. - Modify the two-buffer
@buffer(specs, specs2, ex)macro analogously: allocate both buffers in the caller scope, executeex, then free and clear both buffer variables, again withoutletortry...finally. - Add a v0.2.2 entry to
CHANGELOG.mddocumenting that the scope-related changes to@buffer(use ofletandtryblocks) have been reverted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/mbuffer.jl | Adjusts both @buffer macro definitions to avoid let/try...finally blocks by assigning buffers directly in the caller scope, then freeing and nulling the buffer variables. |
| CHANGELOG.md | Adds release notes for v0.2.2 describing the reversion of the earlier @buffer changes that caused scope issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $(esc(buf)) = MAllocBuffer{$(esc(T))}($(esc(len))) | ||
| $(esc(ex)) | ||
| free!($(esc(buf))) | ||
| $(esc(buf)) = nothing |
There was a problem hiding this comment.
Removing the let/try...finally structure here means free! is no longer guaranteed to run if ex throws, so MAllocBuffer's underlying memory will leak on exceptions (there is no finalizer on MAllocBuffer, and free! is the only place that calls Libc.free). Please restore exception-safe cleanup (e.g., via a try...finally or equivalent pattern that still fixes the scope issue) so that @buffer truly frees the buffer after use even on error.
| $(esc(buf)) = MAllocBuffer{$(esc(T))}($(esc(len))) | ||
| $(esc(buf2)) = MAllocBuffer{$(esc(T2))}($(esc(len2))) | ||
| $(esc(ex)) | ||
| free!($(esc(buf2))) | ||
| free!($(esc(buf))) | ||
| $(esc(buf2)) = nothing | ||
| $(esc(buf)) = nothing |
There was a problem hiding this comment.
As with the single-buffer macro, dropping the let/try...finally wrapper means that if ex throws, neither free!($(esc(buf2))) nor free!($(esc(buf))) will run, leaking both MAllocBuffer instances. Consider reinstating an exception-safe cleanup pattern that still avoids the scope issues you observed, so both buffers are always freed even when ex fails.
Fixes scope issues in
@buffer