Create a LocalVector with capacity. #37370
Conversation
| } else { | ||
| capacity <<= 1; | ||
| } | ||
| data = (T *)memrealloc(data, capacity); |
There was a problem hiding this comment.
Should we care about non-trivial move/copy constructors?
There was a problem hiding this comment.
I think the compiler macro below handles it
There was a problem hiding this comment.
memralloc doesn't. Talking about move/copy here (for all elements being moved due to the realloc), not construct ;) or maybe it's ok and even STL doesn't do it either, I don't know
There was a problem hiding this comment.
STL does not do either, it's common in this case.
| T *data = nullptr; | ||
|
|
||
| public: | ||
| _FORCE_INLINE_ void push_back(T p_elem) { |
| sort_custom<_DefaultComparator<T>>(); | ||
| } | ||
|
|
||
| void ordered_insert(T p_val) { |
There was a problem hiding this comment.
If the goal of this function is to do ordered insert in a sorted array, shouldn't it better use a binary search to find the insertion point? It would assume the array is sorted though, so I wonder why such method would be part of this class
There was a problem hiding this comment.
Depends, for large arrays binary search is faster, but for smaller ones, iterating is faster
| data[count].~T(); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
An unordered O(1) remove is also very useful too, i.e.
remove_unordered(U p_index)
{
// swap p_index element with last
// remove last element so no copying needed
}
There was a problem hiding this comment.
might be nicer to have this be default remove behavior. It seems like more often unordered data is suitable for vectors.
I think an explicit OrderedLocalVector class would be useful, in addition to having a shared interface for Vector and LocalVector so then you can choose behavior and optimization with a consistent API for the data structures.
There was a problem hiding this comment.
I disagree that this should be default. Ordered remove is de-facto standard, making unordered default would create confusion. Besides, I even do such unordered removal explicitely all the time because it often matters what this actually does (in a loop where you may remove elements, for example).
| } | ||
| } | ||
|
|
||
| _FORCE_INLINE_ U size() const { return count; } |
There was a problem hiding this comment.
Could possibly do with a shrink_to_fit() or compact() call to free memory no longer required? As is, it looks like the only way to free memory is to delete the vector.
| } | ||
|
|
||
| void remove(U p_index) { | ||
| ERR_FAIL_UNSIGNED_INDEX(p_index, count); |
There was a problem hiding this comment.
In the case of POD types, these 'mass movements' may be quicker with memmove / memcpy. I'm not sure the compiler will be able to substitute these automatically (not tested though).
| } | ||
| count = p_size; | ||
| } | ||
| } |
There was a problem hiding this comment.
It could be worth adding an additional mechanism for access without bounds checks, for performance critical code (e.g. renderer). In some situations the bounds check can result in considerable slowdown. (That's assuming CRASH_BAD_UNSIGNED_INDEX is being called in release too, if not, then this is less pressing)
There was a problem hiding this comment.
I agree it's not a big deal if bound checks are removed only on release.
|
Force pushed a rebased commit to fix CI build. |
|
Is it a good idea to name this type based on its intended usage rather than its specific functionality? Might be better as "FixedVector" or something since it has a fixed capacity. |
|
@aaronfranke it doesn't have fixed capacity. |
|
superseded by #38386 |
Will eventually use for some things I have planned, in the meantime feel free to review.
Currently Vector<> is used for most things because it has copy on write and you can pass it safely around. For some uses where you want to keep the array local, having a non-cow version with capacity is more useful because of performance and improved memory utilization.
Fixes #24731