Skip to content
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

Improve Container Types #180

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Improve Container Types #180

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
4756017
Adjust file paths
Mozz3d Feb 7, 2025
880f4bd
Improve Container Types
Mozz3d Feb 7, 2025
b6f4513
Fix include path
Mozz3d Feb 7, 2025
937f56d
Adjustments
Mozz3d Feb 7, 2025
6080dc9
Several improvements & adjustments
Mozz3d Feb 11, 2025
00a2473
Fix mistake & ArrayIterator crediting
Mozz3d Feb 11, 2025
3ae9d42
Fix errors
Mozz3d Feb 11, 2025
9a904b4
Fix compiler warning
Mozz3d Feb 11, 2025
ca50d24
Several adjustments
Mozz3d Feb 14, 2025
52c2085
Move SortedArray.hpp
Mozz3d Feb 14, 2025
8d95b35
Adjust include in TweakDB.hpp
Mozz3d Feb 14, 2025
b837509
Fixes and cleanup
Mozz3d Feb 14, 2025
4a0fa19
More adjustments
Mozz3d Feb 14, 2025
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
214 changes: 132 additions & 82 deletions include/RED4ext/Containers/DynArray.hpp
View file
Open in desktop
wopss marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <cassert>
#include <cstdint>
#include <functional>
#include <memory>
#include <type_traits>

#include <RED4ext/Common.hpp>
Expand All @@ -19,6 +20,11 @@ namespace Memory
{
struct IAllocator;
}
template<typename T>
concept ContainerType = requires(T t) {
{ t.begin() } -> std::input_iterator;
{ t.end() } -> std::input_iterator;
};

template<typename T>
struct DynArray
Expand Down Expand Up @@ -47,56 +53,85 @@ struct DynArray
DynArray(std::initializer_list<ValueType> aItems, Memory::IAllocator* aAllocator = nullptr)
: DynArray(aAllocator)
{
Reserve(aItems.size());
Assign(aItems);
}

for (const auto& item : aItems)
{
EmplaceBack(item);
}
DynArray(uint32_t aSize, Memory::IAllocator* aAllocator = nullptr)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
DynArray(uint32_t aSize, Memory::IAllocator* aAllocator = nullptr)
DynArray(SizeType aSize, Memory::IAllocator* aAllocator = nullptr)

: DynArray(aAllocator)
{
Resize(aSize);
}

DynArray(const DynArray& aOther)
: DynArray(aOther.GetAllocator())
{
Reserve(aOther.size);
CopyFrom(aOther);
Assign(aOther.begin(), aOther.end());
}

DynArray(DynArray&& aOther) noexcept
: entries(aOther.Data())
, size(aOther.Size())
, capacity(aOther.Capacity())
{
aOther.entries = reinterpret_cast<Pointer>(aOther.GetAllocator());
aOther.capacity = 0;
aOther.size = 0;
}

template<ContainerType TContainer>
DynArray(const TContainer& aContainer, Memory::IAllocator* aAllocator = nullptr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be done here for example

https://github.com/wopss/RED4ext.SDK/pull/180/files/937f56dff17f36dd9f21867b8d0947685f204709..6080dc94c662a07d0bc97bcc2834c5072fd7c2c2#r1951944671

: DynArray(aAllocator)
{
Assign(aContainer.begin(), aContainer.end());
}

template<ContainerType TContainer>
DynArray(TContainer&& aContainer, Memory::IAllocator* aAllocator = nullptr)
: DynArray(aAllocator)
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a valid use case for this? I can't think of any, and you will have to handle special cases, such as Map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave some of my reasons in discord, but yes I think so. Also the template using the ContainerType concept to enforce what exactly can be used here, so things like Map aren't a concern afaict

Copy link
Owner

Choose a reason for hiding this comment

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

Why not do something like:

template<typename InputIt>
requires std::input_iterator<InputIt>
DynArray(InputIt aFirst, InputIt aLast, Memory::IAllocator* aAllocator = nullptr)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I have a habit of over-complicating things when I haven't slept :)
Thank you

Copy link
Collaborator

Choose a reason for hiding this comment

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

You dont have to use requires in that case, just template <std::input_iterator InputIt> is enough, maybe can be even included in the signature itself, getting rid of the template keyword alltogether if you do not need to do something with the type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And looking at the code now, think you should be able to do just that.

DynArray(std::input_iterator auto aFirst, std::input_iterator auto aLast, Memory::IAllocator* aAllocator = nullptr)

Unsure if it is wanted to be passed by value though, need to look at the implementation in full finally... If you need to pass by reference in the simplified case, just put & after the auto in the above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can actually do sth like that in multiple places from quick glance, you dont seem to be accessing the type explicitly in many of them. Using template keyword in such cases is a bit of a waste.

Copy link
Collaborator

@WSSDude WSSDude Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, rereading my comments, you may not want to do the simplified case without template in the end here as it would match 2 distinct input iterators on input... Still could be done in some other places.

This variant should be used here to ensure same type. #180 (comment)

{
MoveFrom(std::move(aOther));
Assign(std::make_move_iterator(aContainer.begin()), std::make_move_iterator(aContainer.end()))
}

~DynArray()
{
if (capacity)
Clear();
auto allocator = *reinterpret_cast<Pointer*>(GetAllocator());
reinterpret_cast<Memory::IAllocator*>(&allocator)->Free(entries);
entries = allocator;
capacity = 0;
}

DynArray& operator=(const DynArray& aOther)
{
if (this != std::addressof(aOther))
{
Clear();
auto allocator = *reinterpret_cast<Pointer*>(GetAllocator());
reinterpret_cast<Memory::IAllocator*>(&allocator)->Free(entries);
entries = allocator;
capacity = 0;
Assign(aOther.begin(), aOther.end());
}
return *this;
}

DynArray& operator=(const DynArray& aOther)
DynArray& operator=(DynArray&& aOther)
{
if (this != std::addressof(aOther))
{
Clear();
Reserve(aOther.size);
CopyFrom(aOther);
Assign(std::make_move_iterator(aOther.begin()), std::make_move_iterator(aOther.end()));
Copy link
Owner

Choose a reason for hiding this comment

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

This is having a complexity of O(n), Before it was O(1), do the swap trick.

}
return *this;
}

template<ContainerType TContainer>
DynArray& operator=(const TContainer& aOther)
{
Assign(aOther.begin(), aOther.end());
return *this;
}

DynArray& operator=(DynArray&& aOther)
template<ContainerType TContainer>
DynArray& operator=(TContainer&& aOther)
{
if (this != std::addressof(aOther))
{
Clear();
MoveFrom(std::move(aOther));
Assign(std::make_move_iterator(aOther.begin()), std::make_move_iterator(aOther.end()));
}

return *this;
Expand All @@ -114,26 +149,57 @@ struct DynArray
return Data()[aPos];
}

[[nodiscard]] constexpr Reference At(std::make_signed_t<SizeType> aPos)
template<typename TIterator>
constexpr void Assign(TIterator aBegin, TIterator aEnd)
{
if (aPos < 0)
aPos += Size();
SizeType newSize = std::distance(aBegin, aEnd);

if (aPos < 0 || aPos >= Size())
throw std::out_of_range("DynArray::At out of range");
Clear();
Resize(newSize);
Copy link
Owner

Choose a reason for hiding this comment

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

Assign should not shrink_to_fit.


return Data()[static_cast<SizeType>(aPos)];
if constexpr (std::is_trivially_copyable_v<ValueType>)
{
std::memcpy(Data(), aBegin, newSize * sizeof(ValueType));
}
else if constexpr (std::is_move_constructible_v<ValueType>)
{
std::move(aBegin, aEnd, Data());
}
else
{
std::copy(aBegin, aEnd, Data());
}
}

[[nodiscard]] constexpr ConstReference At(std::make_signed_t<SizeType> aPos) const
constexpr void Assign(std::initializer_list<ValueType> aItems)
{
if (aPos < 0)
aPos += Size();
Assign(aItems.begin(), aItems.end());
}

if (aPos < 0 || aPos >= Size())
constexpr void Assign(SizeType aAmount, ValueType aValue)
{
Clear();
Resize(aAmount);
for (SizeType i = 0; i < aAmount; ++i)
{
Emplace(i, aValue);
}
}

[[nodiscard]] constexpr Reference At(SizeType aPos)
{
if (aPos >= Size())
throw std::out_of_range("DynArray::At out of range");

return Data()[static_cast<SizeType>(aPos)];
return Data()[aPos];
}

[[nodiscard]] constexpr ConstReference At(SizeType aPos) const
{
if (aPos >= Size())
throw std::out_of_range("DynArray::At out of range");

return Data()[aPos];
}

[[nodiscard]] constexpr Iterator Find(ConstReference aValue) noexcept
Expand Down Expand Up @@ -170,9 +236,9 @@ struct DynArray
template<class... TArgs>
void Emplace(Iterator aPosition, TArgs&&... aArgs)
{
SizeType posIdx = capacity ? static_cast<SizeType>(aPosition - begin()) : 0;
SizeType posIdx = Capacity() ? static_cast<SizeType>(aPosition - begin()) : 0;
SizeType newSize = Size() + 1;
if (newSize > capacity)
if (newSize > Capacity())
{
Reserve(newSize);
}
Expand All @@ -193,8 +259,10 @@ struct DynArray
if (aSize > Capacity())
Reserve(aSize);

if (aSize < Size())
else if (aSize < Size())
{
std::destroy(begin() + aSize, end());
}

size = aSize;
}
Expand Down Expand Up @@ -229,7 +297,8 @@ struct DynArray

void Clear() noexcept
{
std::destroy(begin(), end());
if (capacity)
std::destroy(begin(), end());
Copy link
Owner

Choose a reason for hiding this comment

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

No need for the check. std::destory is well defined in this case.

This is more like a comment, not a request for change, so if you want feel free to keep it.

size = 0;
}

Expand All @@ -240,6 +309,9 @@ struct DynArray

auto newCapacity = CalculateGrowth(aCount);
SetCapacity(newCapacity);

if (aCount > Size())
std::uninitialized_default_construct(begin() + aCount, end());
}

void ShrinkToFit()
Expand All @@ -265,29 +337,6 @@ struct DynArray
}

#pragma region STL
[[nodiscard]] constexpr Reference Front()
{
assert(!Empty());
return Data()[0];
}

[[nodiscard]] constexpr ConstReference Front() const
{
assert(!Empty());
return Data()[0];
}

[[nodiscard]] constexpr Reference Back()
{
assert(!Empty());
return Data()[Size() - 1];
}

[[nodiscard]] constexpr ConstReference Back() const
{
assert(!Empty());
return Data()[Size() - 1];
}
#pragma region Iterator
[[nodiscard]] constexpr Iterator Begin() noexcept
{
Expand Down Expand Up @@ -390,7 +439,29 @@ struct DynArray
return rend();
}
#pragma endregion
#pragma endregion
[[nodiscard]] constexpr Reference Front()
{
assert(!Empty());
return Data()[0];
}

[[nodiscard]] constexpr ConstReference Front() const
{
assert(!Empty());
return Data()[0];
}

[[nodiscard]] constexpr Reference Back()
{
assert(!Empty());
return Data()[Size() - 1];
}

[[nodiscard]] constexpr ConstReference Back() const
{
assert(!Empty());
return Data()[Size() - 1];
}

[[nodiscard]] constexpr bool Empty() const noexcept
{
Expand All @@ -416,6 +487,7 @@ struct DynArray
{
return size;
}
#pragma endregion

T* entries; // 00
uint32_t capacity; // 08
Expand Down Expand Up @@ -455,14 +527,6 @@ struct DynArray
return (std::max)(aNewSize, geometric);
}

void CopyFrom(const DynArray& aOther)
{
for (uint32_t i = 0; i != aOther.size; ++i)
{
PushBack(aOther[i]);
}
}

void SetCapacity(SizeType aNewCapacity)
{
if (aNewCapacity < size)
Expand All @@ -477,20 +541,6 @@ struct DynArray
static UniversalRelocFunc<func_t> func(Detail::AddressHashes::DynArray_Realloc);
func(this, aNewCapacity, sizeof(ValueType), alignment >= 8 ? alignment : 8, nullptr);
}

void MoveFrom(DynArray&& aOther)
{
entries = aOther.Data();
capacity = aOther.Capacity();
size = aOther.Size();

if (aOther.Capacity())
{
aOther.entries = *reinterpret_cast<Pointer*>(aOther.GetAllocator());
aOther.capacity = 0;
aOther.size = 0;
}
}
};
RED4EXT_ASSERT_SIZE(DynArray<void*>, 0x10);
RED4EXT_ASSERT_OFFSET(DynArray<void*>, capacity, 0x8);
Expand Down
Loading
Loading