Add `back()` and `pop_back()` to `LocalVector`
~~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 anullptrif 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 checkingnullptrto 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.
Isn't this where the CRASH macros would be used? Like https://github.com/godotengine/godot/blob/1f47e4c4e3a09a422e96880a7918d986dd575a63/core/templates/local_vector.h#L173 does it for out of bounds.
Sounds great, I'll try this.
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
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).
So this PR will remain on hold until someone implements something and really needs these functions for something they are implementing
Does harmonizing
LocalVectorwithListsuffice 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 https://github.com/godotengine/godot-proposals/issues/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
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 :)
Now we have #100433, can this be considered useful? 👀
I have some ongoing changes that use this, will submit this along with those.