Skip to content

test realloc behaviour and allocator equality #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pknowles merged 1 commit into from
Jun 3, 2024
Merged

test realloc behaviour and allocator equality #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions include/decodeless/allocator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,16 @@ class linear_memory_resource {
}

// Returns a pointer to the arena/parent allocation.
void* data() const { return reinterpret_cast<void*>(m_begin); }
[[nodiscard]] void* data() const { return reinterpret_cast<void*>(m_begin); }
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

Ensure proper documentation for the data() method.

Adding a brief comment describing the purpose and usage of the data() method would enhance code readability and maintainability.


// Returns the total number of bytes allocated within the arena
size_t size() const { return m_next - reinterpret_cast<uintptr_t>(m_begin); }
[[nodiscard]] size_t size() const { return m_next - reinterpret_cast<uintptr_t>(m_begin); }

// Returns the size of the arena/parent allocation
size_t capacity() const { return m_end - reinterpret_cast<uintptr_t>(m_begin); }
[[nodiscard]] size_t capacity() const { return m_end - reinterpret_cast<uintptr_t>(m_begin); }

// Provide public access to parent allocator. Primarily used for testing.
[[nodiscard]] ParentAllocator& parent() { return m_parent; }
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ensure proper documentation for the parent() method.

Adding a brief comment describing the purpose and usage of the parent() method would enhance code readability and maintainability.

Copy link
Contributor Author

@pknowles pknowles Jun 3, 2024

Choose a reason for hiding this comment

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

What would you suggest? For reference, this is the equivalent of e.g. upstream_resource in std::pmr::monotonic_buffer_resource.

Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

@pknowles, based on the reference to std::pmr::monotonic_buffer_resource::upstream_resource, here's a suggested documentation comment for the parent() method:

/**
 * Returns a reference to the parent allocator.
 * This method provides access to the underlying allocator used by this `linear_memory_resource`,
 * similar to how `upstream_resource` provides access to the upstream memory resource in `std::pmr::monotonic_buffer_resource`.
 * It is primarily used for testing and debugging purposes, where direct access to the underlying allocator is necessary.
 */
[[nodiscard]] ParentAllocator& parent() { return m_parent; }

This comment explains the purpose and usage of the parent() method, aligning it with the standard library's terminology and functionality. Let me know if this meets your expectations or if there are any adjustments you'd like to make!


private:
ParentAllocator m_parent;
Expand Down Expand Up @@ -176,30 +179,46 @@ class memory_resource_ref {
return m_resource->deallocate(static_cast<void*>(p), n);
}

bool operator==(const memory_resource_ref& other) const {
[[nodiscard]] constexpr T* reallocate(T* ptr, std::size_t bytes)
requires realloc_memory_resource<MemoryResource>
{
return static_cast<T*>(m_resource->reallocate(ptr, bytes));
}
Comment on lines +182 to +186
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

Ensure proper documentation for the reallocate method.

Adding a brief comment describing the purpose and usage of the reallocate method would enhance code readability and maintainability.


[[nodiscard]] constexpr size_t max_size() const
requires has_max_size<MemoryResource>
{
return m_resource->max_size();
}
Comment on lines +188 to +192
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

Ensure proper documentation for the max_size method.

Adding a brief comment describing the purpose and usage of the max_size method would enhance code readability and maintainability.

Comment on lines +182 to +192
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

Ensure proper documentation for new methods reallocate and max_size.

Adding a brief comment describing the purpose and usage of these methods would enhance code readability and maintainability.


template <trivially_destructible U>
[[nodiscard]] bool operator==(const memory_resource_ref<U, MemoryResource>& other) const {
return m_resource == other.m_resource;
}

bool operator!=(const memory_resource_ref& other) const {
template <trivially_destructible U>
[[nodiscard]] bool operator!=(const memory_resource_ref<U, MemoryResource>& other) const {
Comment on lines +194 to +200
Copy link

@coderabbitai coderabbitai bot Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Add unit tests for the templated equality and inequality operators.

The changes to the equality and inequality operators are significant. Would you like me to help by adding unit tests to ensure their correct behavior?

Add unit tests for the templated equality and inequality operators.

The changes to the equality and inequality operators are significant. Would you like me to help by adding unit tests to ensure their correct behavior?

Copy link
Contributor Author

@pknowles pknowles Jun 3, 2024

Choose a reason for hiding this comment

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

Yes

Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

@pknowles, here are the unit tests for the templated equality and inequality operators in the memory_resource_ref class. These tests ensure that the operators function correctly across different types and scenarios:

#include <gtest/gtest.h>
#include "allocator.hpp"

class TestResource : public decodeless::memory_resource {
public:
    void* allocate(size_t size, size_t alignment) override {
        return ::operator new(size);
    }

    void deallocate(void* p, size_t size) override {
        ::operator delete(p);
    }
};

TEST(MemoryResourceRef, Equality) {
    TestResource res1, res2;
    decodeless::memory_resource_ref<int, TestResource> ref1(res1);
    decodeless::memory_resource_ref<float, TestResource> ref2(res1);
    decodeless::memory_resource_ref<int, TestResource> ref3(res2);

    // Test equality of the same resource type and instance
    EXPECT_TRUE(ref1 == ref1);
    // Test equality of different resource types but same instance
    EXPECT_TRUE(ref1 == ref2);
    // Test inequality of different instances
    EXPECT_FALSE(ref1 == ref3);
}

TEST(MemoryResourceRef, Inequality) {
    TestResource res1, res2;
    decodeless::memory_resource_ref<int, TestResource> ref1(res1);
    decodeless::memory_resource_ref<float, TestResource> ref2(res1);
    decodeless::memory_resource_ref<int, TestResource> ref3(res2);

    // Test inequality of the same resource type and instance
    EXPECT_FALSE(ref1 != ref1);
    // Test inequality of different resource types but same instance
    EXPECT_FALSE(ref1 != ref2);
    // Test inequality of different instances
    EXPECT_TRUE(ref1 != ref3);
}

These tests use a simple TestResource class that implements the memory_resource interface. They check the behavior of the equality and inequality operators under different conditions, ensuring that your implementation behaves as expected. Let me know if there's anything else you need!

return m_resource != other.m_resource;
}

resource_type& resource() const { return *m_resource; }
// Public access required for rebind
[[nodiscard]] resource_type& resource() const { return *m_resource; }
Copy link

@coderabbitai coderabbitai bot Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mark resource() method as const.

The resource() method returns a reference and does not modify the object state, so it should be marked as const to ensure it can be called on const objects.

Copy link
Contributor Author

@pknowles pknowles Jun 3, 2024

Choose a reason for hiding this comment

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

This is required by the rebind constructor that converts between allocators that create different types. The only alternative would probably be declaring other allocator types as friends. I'm not sure which is more widely done.

Copy link

@coderabbitai coderabbitai bot Jun 3, 2024

Choose a reason for hiding this comment

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

@pknowles, your approach to mark the resource() method as const is indeed aligned with best practices in C++. The const qualifier is essential for methods that do not alter the state of the object, allowing them to be used in contexts where object immutability must be preserved, such as in your case with the rebind constructor for different allocator types.

Regarding the alternative of declaring other allocator types as friends, this is generally less common for this purpose. Using friend declarations can lead to tighter coupling between classes, which might complicate maintenance and reduce encapsulation. The use of const is a cleaner and more widely accepted approach in modern C++ design, especially in template and generic programming where object state integrity is crucial.

Your current implementation is well-justified and adheres to good C++ practices. If you have any more questions or need further clarification, feel free to ask!


// Needed by msvc
template <class U>
struct rebind {
using other = memory_resource_ref<U, MemoryResource>;
};

protected:
private:
resource_type* m_resource;
};

// STL compatible allocator with an implicit linear_memory_resource memory
// resource. The need for this emphasizes why std::pmr is a thing - the
// MemoryResource would ideally not affect the type.
template <trivially_destructible T, memory_resource MemoryResource = linear_memory_resource<>>
using linear_allocator = memory_resource_ref<T, MemoryResource>;
template <trivially_destructible T,
memory_resource_or_allocator ParentAllocator = std::allocator<std::byte>>
using linear_allocator = memory_resource_ref<T, linear_memory_resource<ParentAllocator>>;

} // namespace decodeless
75 changes: 75 additions & 0 deletions test/src/allocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,43 @@ struct NullAllocator {
bool allocated = false;
};

struct ReallocNullAllocator {
using value_type = std::byte;
value_type* allocate(std::size_t n) {
EXPECT_EQ(size, 0);
size = n;
return nullptr;
}
value_type* reallocate(value_type* p, std::size_t n) noexcept {
EXPECT_EQ(p, nullptr);
EXPECT_GT(size, 0);
size = n;
return nullptr;
}
void deallocate(value_type* p, std::size_t n) noexcept {
EXPECT_GT(size, 0);
(void)p;
(void)n;
}
size_t size = 0;
};

static_assert(realloc_allocator<ReallocNullAllocator>);

struct ReallocNullMemoryResource {
void* allocate(std::size_t, std::size_t) { return nullptr; }
void* reallocate(void*, std::size_t, std::size_t) noexcept { return nullptr; }
void deallocate(void*, std::size_t) noexcept {}
};

static_assert(realloc_memory_resource<ReallocNullMemoryResource>);
static_assert(realloc_allocator<memory_resource_ref<std::byte, ReallocNullMemoryResource>>,
"linear_allocator should enable reallocate when possible");
static_assert(!realloc_allocator<linear_allocator<std::byte, std::allocator<std::byte>>>,
"std::allocator does not reallocate");
static_assert(!realloc_memory_resource<linear_memory_resource<ReallocNullAllocator>>,
"linear memory does not reallocate, only its parent");

TEST(Allocate, Object) {
linear_memory_resource<NullAllocator> memory(23);

Expand Down Expand Up @@ -96,6 +133,44 @@ TEST(Allocate, Initialize) {
EXPECT_EQ(span3[2], 5);
}

TEST(Allocate, Realloc) {
linear_memory_resource<ReallocNullAllocator> alloc(4);
EXPECT_EQ(alloc.parent().size, 4);
(void)alloc.allocate(sizeof(int), alignof(int));
EXPECT_EQ(alloc.parent().size, 1 * sizeof(int));
(void)alloc.allocate(sizeof(int), alignof(int));
EXPECT_EQ(alloc.parent().size, 2 * sizeof(int));

// Allocate exact size for allocations exceeding double capacity
(void)alloc.allocate(sizeof(int) * 1000, alignof(int));
EXPECT_EQ(alloc.parent().size, 1002 * sizeof(int));

// Doubling of capacity for allocations under double existing capacity
(void)alloc.allocate(sizeof(int), alignof(int));
EXPECT_EQ(alloc.parent().size, 2004 * sizeof(int));

// Truncate should truncate the parent allocator
alloc.truncate();
EXPECT_EQ(alloc.parent().size, 1003 * sizeof(int));
}

TEST(Allocate, Equality) {
linear_memory_resource<NullAllocator> r0(4);
linear_memory_resource<NullAllocator> r1(4);
linear_memory_resource<std::allocator<std::byte>> r2(4);
linear_allocator<int, NullAllocator> a0(r0);
linear_allocator<int, NullAllocator> a1(r1);
linear_allocator<int, std::allocator<std::byte>> a2(r2);
linear_allocator<int, NullAllocator> c0(a0);
linear_allocator<int, NullAllocator> c1(a1);
linear_allocator<int, std::allocator<std::byte>> c2(a2);
EXPECT_EQ(a0, c0);
EXPECT_EQ(a1, c1);
EXPECT_EQ(a2, c2);
EXPECT_NE(a0, c1);
EXPECT_NE(a1, c0);
}

// Relaxed test case for MSVC where the debug vector allocates extra crap
TEST(Allocate, VectorRelaxed) {
linear_memory_resource alloc(100);
Expand Down