Add back() and pop_back() to LocalVector

[YYF233333]

Follow up #99936.

Having some difficulties so I upload this as draft for discussion.

Which type should LocalVector::back() return?

• Return T&.
  Simplify caller's code, but not that compitable with the semantic of reference, since one can get a nullptr if container is empty. What should be returned if empty? (We do have bound check so this is unlikely to happen)
• Return T *.
  Shift the responsibility of checking nullptr to caller, resulting in a lot of explicit dereference. Also not sure if deref a pointer has the same effect as deref a reference for complex types.
• Return Option<T>.
  Too rusty I think :)

What's your opinion?

Split the LocalVector change and List change to keep PR small and easy to review.

This one add back() and pop_back() method for LocalVector so it's capable as a stack. It will be used to replace List based stack in following PR.

[Spartan322]

Isn't this where the CRASH macros would be used? Like

godot/core/templates/local_vector.h

Line 173 in 1f47e4c

CRASH_BAD_UNSIGNED_INDEX(p_index, count);

does it for out of bounds.

[YYF233333]

Sounds great, I'll try this.

[Ivorforce]

Looks good to me! Useful functions.

[Ivorforce]

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.

[YYF233333]

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

[Ivorforce]

Why not use CRASH_BAD_UNSIGNED_INDEX here too?

[YYF233333]

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

[Ivorforce]

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

[clayjohn]

This looks great! But just a heads up that, as a rule, we don't merge things into core without a tangible benefit (i.e. fixing a bug, needed for a new piece of code, improving performance).

In line with our best practices document the problem must always come before the solution. In other words, we don't add new things just because we can, or just because we think it might be useful to someone in the future. We add things when they become useful right now.

So this PR will remain on hold until someone implements something and really needs these functions for something they are implementing

[Ivorforce]

So this PR will remain on hold until someone implements something and really needs these functions for something they are implementing

Does harmonizing LocalVector with List suffice as a reason to add the functions? (although notably, List.back() returns a pointer, not a reference).

[clayjohn]

So this PR will remain on hold until someone implements something and really needs these functions for something they are implementing

Does harmonizing LocalVector with List suffice as a reason to add the functions? (although notably, List.back() returns a pointer, not a reference).

No. It needs to be something with a tangible benefit.

The purpose of godotengine/godot-proposals#5144 isn't to just make Vector and LocalVector look the same. It's to allow us to avoid copying arrays as we pass them around. Avoiding copy operations is an example of a tangible benefit

[YYF233333]

This looks great! But just a heads up that, as a rule, we don't merge things into core without a tangible benefit (i.e. fixing a bug, needed for a new piece of code, improving performance).

Note the title change, it is a prerequisite to replace some List. I'm a bit busy now so push this for review first. Will implement remain work if I get time.

It is fine if you don't want to merge it now, but do make a consensus over these methods. I don't want to end up like #97777 again :)

[YYF233333]

Now we have #100433, can this be considered useful? 👀

Dismissing as my checkmark turned green now, and I don't want to misrepresent the state of the PR.
Personally, I think we'll add back() and pop_back sooner or later anyway, but we may also just add it when it's needed.

[YYF233333]

I have some ongoing changes that use this, will submit this along with those.