Avoid unnecessary copies of container types

[aaronfranke]

This PR was made by doing the following:

• Change the copy constructor of all container types to explicit (except Vector).
• Fix all of the places where that broke compiling things, including manually checking if we really need to copy there.
• Keep only the changes that avoid copying.
• Discard the changes that make copying explicit and return the copy constructors to normal.

This PR changes behavior by avoiding copies, which should only improve performance with no negative consequences. For each change I checked if it was good. However, it is perfectly possible that I made mistakes.

After this PR, we can follow-up with either making the copy constructor explicit or adding duplicate, see the discussion in PR #107377 for more information.

[aaronfranke]

I added a const version of List::find to support using find in const contexts.

[aaronfranke]

Yes, this was a copy before. I assume this was done as a separate variable to avoid the problem of commas inside macros, but there is a solution: Wrap the macro arguments in parenthesis. So in this case, the value returned by this error macro is (HashMap<Vector2i, TileMapCell>()).

[Ivorforce]

Change the copy constructor of all container types to explicit (except Vector).

I'm not seeing this change, did you push this?

[aaronfranke]

@Ivorforce No, please see the part of the PR description where I wrote "and return the copy constructors to normal". It's a list of things I did in order, not a list of the final changes in the PR, sorry for the confusion.

[Ivorforce]

@Ivorforce No, please see the part of the PR description where I wrote "and return the copy constructors to normal". It's a list of things I did in order, not a list of the final changes in the PR, sorry for the confusion.

Aha, makes sense. Why did you return the constructors to implicit after your fixes?

[aaronfranke]

@Ivorforce Because the rest of the changes span 100 files and I want to separate them for improved reviewability. The existing codebase of Godot has many places where implicit copy constructor is used, which all had to be changed.

[Ivorforce]

Alright, that makes sense. Thanks for clearing that up!

[Ivorforce]

This is a great effort. Those implicit copies keep haunting us, and we really should be getting rid of them.

Unfortunately, this change is not without risk. Taking a reference instead of making a copy makes the caller vulnerable to changes during execution of the code that uses these variables. If the underlying data is modified, this could lead to crashes, data corruption, or subtle bugs.
Since this change is motivated and informed by a syntactical change (making the constructors explicit), rather than a profiler, I think there's too much risk of regression to make this a blanket change. I might be swayed if you go through all of the code, and make absolutely sure there is no risk of mutation regressions, but I'm not sure if this is worth it (since we don't know if any of this is in performance critical code).

If I were to make this change, I would simply make the call explicit syntactically (via e.g. List(other_list)), to keep the syntactic change agnostic to possible changes in behavior. If the code pops up in a profiler in the future, it can more easily be fixed, because the copy is explicit and therefore an obvious target for optimization.

[aaronfranke]

@Ivorforce That's why I think such a change should be done early in the dev cycle, to increase the chances that any problems are caught before a release happens. Since 4.5 was just released, we have plenty of time to test before 4.6.

[Ivorforce]

@Ivorforce That's why I think such a change should be done early in the dev cycle, to increase the chances that any problems are caught before a release happens. Since 4.5 was just released, we have plenty of time to test before 4.6.

That's true, but I personally wouldn't feel comfortable risking regressions over this without any tangible benefit (i.e. profiling or benchmarking performance improvements).

[aaronfranke]

@Ivorforce Understandable, but then I am left stuck between a rock and a hard place:

1. I want to allow new code to be explicit when duplication is required, and gradually migrate to this, so I proposed adding the duplicate function.
2. You stated that we can't do that unless we migrate all existing copy constructors to duplicate for consistency.
3. Migrating all existing copy constructor uses to either explicit or duplicate implies that everywhere we are currently using a copy constructor will change, even when the code author did not intend that.
4. I am hesitant to change the visible intention of existing code, especially to something I am confident is incorrect (using copy constructors where none was intended).

This really just leads me back to option 1 again, adding the duplicate function without deleting the copy constructors, which is risk-free (unlike this PR) and does not destroy the intent of existing code (unlike blanket changing everything). I can update PR #107377 to add this to the rest of the container types if this is desired.

Alternatively, I would be happy with whatever course of action you propose, if you would like to make a PR yourself.