Allocator docs#32
Conversation
|
Warning Review limit reached
More reviews will be available in 25 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces comprehensive allocator documentation and enforces error handling in the allocator API. A new ChangesAllocator Documentation and Error Enforcement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/allocators.md (2)
342-344: ⚡ Quick winImprove phrasing for clarity.
The phrase "return allocations into" is somewhat awkward.
📝 Suggested rewording
-Allocators which don't themselves produce memory (e.g. a `PageAllocator`) ***must*** -hold onto another allocator from which they ask for more memory or a buffer from -which they return allocations into (e.g. a `FixedAllocator`). +Allocators which don't acquire memory directly from the OS (e.g., those other than `PageAllocator`) ***must*** +hold onto another allocator from which they request more memory or a buffer from +which they supply allocations (e.g. a `FixedAllocator`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/allocators.md` around lines 342 - 344, The sentence using "return allocations into" is awkward; update the wording so it clearly expresses that non-producing allocators must retain another allocator or buffer that they request memory from and to which allocations are returned—for example, change the clause to something like "must hold onto another allocator from which they request memory or a buffer to which allocations are returned (e.g. a `FixedAllocator`)" and ensure `PageAllocator` and `FixedAllocator` remain referenced for clarity.
300-300: ⚡ Quick winClarify filename reference.
The shell brace expansion syntax
allocator.cpp{,m}may not be immediately clear to all readers.📝 Suggested clarification
-provide the appropriate `nil` stub function (found in allocator.cpp{,m}). There +provide the appropriate `nil` stub function (found in allocator.cpp or allocator.cppm). There🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/allocators.md` at line 300, The notation allocator.cpp{,m} is ambiguous; replace it with explicit filenames to avoid confusion—e.g., write "allocator.cpp and allocator.m" (or "allocator.cpp and allocator.mm" if you meant Objective‑C++), and update the surrounding text so readers clearly see both stub files referenced instead of the brace‑expansion form.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/allocators.md`:
- Around line 18-21: Finish the incomplete sentence by adding a main clause
explaining how PageAllocator is handled; for example, complete the sentence
after "which, due to its global nature as it integrates with the kernel" to
state that "is accessed via a global variable rather than being instantiated
per-user, so it must be acquired from the appropriate global variable." Update
the text around Allocator and PageAllocator to read as a single complete
sentence referencing the PageAllocator symbol.
In `@engine/native/core/memory/fixedAllocator.test.cpp`:
- Around line 16-17: The test discards alloc.alloc's result and then asserts
alignment on block.data; change it to capture and verify the allocation
succeeded before checking alignment: check the return value from alloc.alloc
(the error/result) with REQUIRE (or EXPECT) to ensure success, then perform the
alignment assertion on block.data; reference alloc.alloc and block to locate the
change.
---
Nitpick comments:
In `@docs/allocators.md`:
- Around line 342-344: The sentence using "return allocations into" is awkward;
update the wording so it clearly expresses that non-producing allocators must
retain another allocator or buffer that they request memory from and to which
allocations are returned—for example, change the clause to something like "must
hold onto another allocator from which they request memory or a buffer to which
allocations are returned (e.g. a `FixedAllocator`)" and ensure `PageAllocator`
and `FixedAllocator` remain referenced for clarity.
- Line 300: The notation allocator.cpp{,m} is ambiguous; replace it with
explicit filenames to avoid confusion—e.g., write "allocator.cpp and
allocator.m" (or "allocator.cpp and allocator.mm" if you meant Objective‑C++),
and update the surrounding text so readers clearly see both stub files
referenced instead of the brace‑expansion form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 739ab1d7-8938-4208-85fc-66f75eb20283
📒 Files selected for processing (3)
docs/allocators.mdengine/native/core/memory/allocator.cppmengine/native/core/memory/fixedAllocator.test.cpp
| The current `Allocator` design separates their own resource acquisition and | ||
| initialization. The only current exception is the `PageAllocator`, which, due to | ||
| its global nature as it integrates with the kernel. The `PageAllocator` can be | ||
| acquired from the appropriate global variable. |
There was a problem hiding this comment.
Complete the sentence about PageAllocator.
The sentence on lines 18-21 is incomplete. The clause "which, due to its global nature as it integrates with the kernel" is a dependent clause that lacks a main clause.
📝 Suggested completion
-The current `Allocator` design separates their own resource acquisition and
-initialization. The only current exception is the `PageAllocator`, which, due to
-its global nature as it integrates with the kernel. The `PageAllocator` can be
-acquired from the appropriate global variable.
+The current `Allocator` design separates their own resource acquisition and
+initialization. The only current exception is the `PageAllocator`, which, due to
+its global nature as it integrates with the kernel, does not require explicit
+initialization. The `PageAllocator` can be acquired from the appropriate global variable.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The current `Allocator` design separates their own resource acquisition and | |
| initialization. The only current exception is the `PageAllocator`, which, due to | |
| its global nature as it integrates with the kernel. The `PageAllocator` can be | |
| acquired from the appropriate global variable. | |
| The current `Allocator` design separates their own resource acquisition and | |
| initialization. The only current exception is the `PageAllocator`, which, due to | |
| its global nature as it integrates with the kernel, does not require explicit | |
| initialization. The `PageAllocator` can be acquired from the appropriate global variable. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/allocators.md` around lines 18 - 21, Finish the incomplete sentence by
adding a main clause explaining how PageAllocator is handled; for example,
complete the sentence after "which, due to its global nature as it integrates
with the kernel" to state that "is accessed via a global variable rather than
being instantiated per-user, so it must be acquired from the appropriate global
variable." Update the text around Allocator and PageAllocator to read as a
single complete sentence referencing the PageAllocator symbol.
2 participants
Simple documentation for allocators and mark
allocas nodiscard.Fixes #31
Summary by CodeRabbit
Documentation
Chores