Skip to content

Refactor Blocked class to meet allocator requirements#351

Merged
rfsaliev merged 1 commit into
mainfrom
rfsaliev/dual-allocator
Jul 17, 2026
Merged

Refactor Blocked class to meet allocator requirements#351
rfsaliev merged 1 commit into
mainfrom
rfsaliev/dual-allocator

Conversation

@rfsaliev

Copy link
Copy Markdown
Member

This pull request refactors the Blocked class template to inherit from its allocator type instead of storing it as a member, and updates the associated test to use a struct-based allocator. This change simplifies allocator handling and improves compatibility with standard allocator patterns.

Core class refactoring:

  • Blocked now inherits from its allocator type (Alloc) instead of storing an Alloc allocator_ member, which simplifies construction, copying, and access to the allocator. The get_allocator() method now returns *this (as an allocator), and constructors have been updated accordingly.

Test improvements:

  • The test for Blocked with an allocator has been updated to use a struct-based allocator (I), which provides a value_type and integer value for testing propagation and compatibility with the new inheritance-based implementation.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors svs::data::Blocked<Alloc> (public header) to inherit from its allocator type rather than storing it as a member, aiming to better align Blocked with allocator conventions while preserving access to BlockingParameters.

Changes:

  • Blocked now derives from Alloc and returns the allocator via *this rather than a stored member.
  • Blocked introduces a value_type alias via std::allocator_traits.
  • The Blocked allocator test switches from an int “allocator” to a struct with value_type.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
include/svs/core/data/simple.h Refactors Blocked to inherit from Alloc and updates constructors/accessors accordingly.
tests/svs/core/data/block.cpp Updates the allocator-propagation test to use a struct-based “allocator” type.

Comment thread include/svs/core/data/simple.h
Comment thread tests/svs/core/data/block.cpp

@yuejiaointel yuejiaointel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, understanding is using class instead of field saves memory because field costs at least 1 byte and class can be empty.

@mergify

mergify Bot commented Jul 16, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@rfsaliev
rfsaliev merged commit 154af0e into main Jul 17, 2026
21 checks passed
@rfsaliev
rfsaliev deleted the rfsaliev/dual-allocator branch July 17, 2026 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants