Allocator docs#32
Conversation
📝 WalkthroughWalkthroughThis PR adds a new documentation file, Docs/allocators.md, describing the allocator-based memory model for native code, including usage rules, allocation guarantees, API conventions, ownership requirements, failure-case examples, and implementation requirements for custom allocators. ChangesAllocator Documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: PoemA rabbit hops through pages new, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
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.
JoltedJon
left a comment
There was a problem hiding this comment.
Looks good, hopefully should help clear any confusion on how to use these since it's a bit strange for most C++ programmers to be programming like this. Hopefully helps trend towards best practices that makes it easier to keep the program fast
|
Needs a rebase |
7732136 to
810a548
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 342-344: Reword the allocator guidance in the allocators
documentation so it no longer uses PageAllocator as an example of an allocator
that must hold another allocator or buffer; update the sentence around the “must
hold onto another allocator” rule to apply only to allocators that are backed by
something else, and adjust the surrounding wording to keep it consistent with
the earlier PageAllocator description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| 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`). No newline at end of file |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't use PageAllocator as the example here.
This says allocators that don't produce memory, including PageAllocator, must hold another allocator or buffer. That conflicts with the earlier description of PageAllocator as the primitive allocator, so the guidance is misleading. Reword this to apply only to allocators that are backed by something else.
♻️ Suggested wording
-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 themselves produce memory and are backed by another allocator (e.g. a `FixedAllocator`) ***must*** hold onto that backing allocator or a buffer from which they return allocations into.📝 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.
| 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 themselves produce memory and are backed by another allocator (e.g. a `FixedAllocator`) ***must*** hold onto that backing allocator or a buffer from which they return allocations into. |
🤖 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, Reword the allocator guidance in
the allocators documentation so it no longer uses PageAllocator as an example of
an allocator that must hold another allocator or buffer; update the sentence
around the “must hold onto another allocator” rule to apply only to allocators
that are backed by something else, and adjust the surrounding wording to keep it
consistent with the earlier PageAllocator description.
Shakai-Dev
left a comment
There was a problem hiding this comment.
Change the name from Docs/allocators.md to Docs/Allocators.md, then it can be merged
Simple documentation for allocators and mark
allocas nodiscard.Fixes #31
Summary by CodeRabbit