Use C++ range iterators for Lists in many situations

[aaronfranke]

With #50284, we now have range iterators. This PR uses them throughout the codebase for List. This PR doesn't replace every single usage of List<>::Element, but it does replace many of them.

It's likely that I made some mistakes, but it would be like finding a needle in a haystack simply because this PR is so big. 99% of this should be good. This took a lot longer than I expected.

[jordo]

I think part of the optimization idea is to not have to construct these temporary lists everywhere. In the above example, we're not gaining anything (a temporary list is still constructed and just used for iteration purposes, although it's ranged now).

The idea here, is we shouldn't need to call d.get_key_list(&keys);. We should just iterate over d using a KeyPair reference. No heap allocations.

[aaronfranke]

Is this implemented? This line doesn't compile:

for (KeyValue<Variant, Variant> &E : d) {

this range-based 'for' statement requires a suitable "begin" function and none was found C/C++(2291)

[jordo]

I have no idea, i just found out reduz implemented range iterators a couple days ago. Doesn't look like it, based on #50284 though. Seems a few more data structures still need the support.

[akien-mga]

Yes the idea is to do the conversion to the next syntax everywhere we can, and then keep working on optimizing usage in the templates directly.

[jordo]

I think part of this refactor should be removing the temporary lists. Yes it's nice to move the actual iterator to a range, but we should remove these temporary lists too. .get_key_list(&keys) is an easy target. The internals of get_key_list(&List) requires a round trip to malloc/free and back for every key. If we can iterate dictionaries and maps with KeyValue pair references, we should be able to eliminate all of these.

[jordo]

TBH, the API (c++ side) for getting a collection of keys from a map shouldn't even be exposed IMO. Iterators should be enough, and if you really need a copy of the keys, it's easy to construct from iterator. If it really must be exposed it should maybe be Vector<String> get_keys() const;. The compiler will move or (copy elicit) the Vector. and with c++17 compilers, copy elision is guaranteed. There's not much reason to pass a reference argument for output in modern c++ these days.

But I honestly wouldn't even include it because it'll encourage it's use over the cost-free iterator. This will bring the codebase inline with other template collections like stdlib stl.

Thoughts @reduz ? Also MUCH appreciation for implementing range iterators. I have huge respect for anyone who takes criticism well. Sometimes I think OSS maintainers have the thickest skin imaginable. 🍻

[reduz]

@jordo The idea is there, but the plan is to do one thing at a time. The first step is to remove all usages of the Element syntax across the codebase. Once that has been accomplished, the next step is to migrate the codebase to more efficient data types depending on usage, which should be for the most part transparent due to the range iterator syntax.

[akien-mga]

Haven't done a thorough review but at a glance it looks good, and seems to compile.

There's still all these now badly named E and F local variables that will need to be renamed to more sensible names at some point... and that's going to be super tedious as it's not something that can be easily automated... But that's for another PR. I see you've already done significant effort to reuse a potential E->get() local variable as the iterator where relevant.

From a quick local compile test, I seem to run into an issue where I can't run projects:

Running: /home/akien/Projects/godot/godot.git/bin/godot.linuxbsd.tools.64 --path /home/akien/Test4.0 --remote-debug tcp://127.0.0.1:6007 --allow_focus_steal_pid 247548 --position 448,240
Error: Can't run project: no main scene defined.
ERROR: Condition "int32_t(p_level) != level" is true.
   at: deinitialize_extensions (core/extension/native_extension_manager.cpp:101)

Project settings seem to be bogus:

I've confirmed that this doesn't happen in 5156703.

[akien-mga]

Did another test build, now it seems to work fine after the last round of fixes. I did not test extensively or on a complex project.

We'll probably need to merge and get it tested in nightly builds to find potential other bugs, we just need to time it properly so that it doesn't overlap too much with other suspicious commits that could also create significant instability.

[akien-mga]

Thanks!