Skip to content

Add back() and pop_back() to LocalVector #99955

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

Closed
YYF233333 wants to merge 1 commit into from
Closed

Add back() and pop_back() to LocalVector #99955

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
18 changes: 18 additions & 0 deletions core/templates/local_vector.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,16 @@ class LocalVector {
return data;
}

T &back() {
CRASH_BAD_UNSIGNED_INDEX(0, count);
return data[count - 1];
}

const T &back() const {
CRASH_BAD_UNSIGNED_INDEX(0, count);
return data[count - 1];
}

// Must take a copy instead of a reference (see GH-31736).
_FORCE_INLINE_ void push_back(T p_elem) {
if (unlikely(count == capacity)) {
Expand All @@ -72,6 +82,14 @@ class LocalVector {
}
}

void pop_back() {
Copy link
Member

@Ivorforce Ivorforce Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's customary to return the value instead of destructing it, so that it can be used in structures like:

while (element = list.pop_pack()) {
}

But this is, unfortunately, not the case with equivalent functions of other list types. So I think it would be better to keep it as you have it, and address this shortcoming for all list types in a separate PR.

Copy link
Contributor Author

@YYF233333 YYF233333 Dec 12, 2024

Choose a reason for hiding this comment

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

I remember std::vector::pop_back return void because of multi-thread concern so I follow this prcatice.

ERR_FAIL_COND(count == 0);
Copy link
Member

@Ivorforce Ivorforce Dec 12, 2024

Choose a reason for hiding this comment

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

Why not use CRASH_BAD_UNSIGNED_INDEX here too?

Copy link
Contributor Author

@YYF233333 YYF233333 Dec 12, 2024

Choose a reason for hiding this comment

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

I remember godot prefer throw error and continue than crash, follow that rule.

Copy link
Member

@Ivorforce Ivorforce Dec 12, 2024

Choose a reason for hiding this comment

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

Ah, right, the other function crashes. Makes sense to me!

count--;
if constexpr (!std::is_trivially_destructible_v<T> && !force_trivial) {
data[count].~T();
}
}

void remove_at(U p_index) {
ERR_FAIL_UNSIGNED_INDEX(p_index, count);
count--;
Expand Down
Loading