Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements import/export functionality for the PoolAllocator class to enable transferring memory blocks and free slots between allocator instances. The changes also modify the allocator's equality semantics and lifecycle management.
- Adds
ExportedAllocstruct and import/export methods for transferring allocator state - Removes copy/move constructors and assignment operators, making allocators non-copyable/movable
- Changes equality comparison to reference-based instead of type-based
- Adds debug helper functions for querying allocator state
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| include/pool_allocator/pool_allocator.h | Adds ExportedAlloc struct, import/export method declarations, removes copy/move operations, and updates equality operators |
| include/pool_allocator/pool_allocator.tcc | Implements constructor changes, import/export functionality, and removes copy/move constructor implementations |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
canlin-zhang
force-pushed
the
multi-thread
branch
from
551abfd to
4f4426a
Compare
|
@eanorige Changes are done as requested. |
| //! Export only the available slots as a vector of pointers. | ||
| //! Warning: This does NOT transfer ownership of the underlying memory blocks. | ||
| //! Do NOT use this function in threads with shorter lifetimes than other threads | ||
| //! accessing objects backed by this allocator. Doing so may lead to use-after-free. |
There was a problem hiding this comment.
Good warning. It may be useful to add to README.md some text that details the intended use of these functions.
There was a problem hiding this comment.
Agreed. Let me change that in the README.md docs.
| std::make_move_iterator(exported.memory_blocks.begin()), | ||
| std::make_move_iterator(exported.memory_blocks.end())); |
There was a problem hiding this comment.
Again, why the move iterators for copying pointers? It's sort-of correct, as our goal is re-homing ownership of these pointers, but I bet it'll cost some compile time and not improve generated code.
There was a problem hiding this comment.
Agreed, I think begin() and end() already serve the purpose well.
|
|
||
| return *this; | ||
| // Clear the imported memory blocks | ||
| exported.memory_blocks = std::vector<pointer>(); |
There was a problem hiding this comment.
Assignment to an empty vector isn't the right way to clear this; better is the clear() method.
There was a problem hiding this comment.
Wait - really? I thought std::move already set the T&& t to an undefined state.
There was a problem hiding this comment.
Yes, but continuing to use a moved-from value has no guaranteed behavior, so it's necessary to clear it so you know what state the vector is in.
2 participants
No description provided.