godot icon indicating copy to clipboard operation
godot copied to clipboard

Add `back()` and `pop_back()` to `LocalVector`

Open YYF233333 opened this issue 1 year ago • 6 comments

~~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.

YYF233333 avatar Dec 03 '24 08:12 YYF233333

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.

Spartan322 avatar Dec 04 '24 20:12 Spartan322

Sounds great, I'll try this.

YYF233333 avatar Dec 05 '24 07:12 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).

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

clayjohn avatar Dec 12 '24 23:12 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).

Ivorforce avatar Dec 13 '24 00:12 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).

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

clayjohn avatar Dec 13 '24 00:12 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).

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 avatar Dec 13 '24 04:12 YYF233333

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

YYF233333 avatar Dec 16 '24 17:12 YYF233333

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

YYF233333 avatar Mar 31 '25 14:03 YYF233333