Add const references detected by clang-tidy

[Rubonnek]

I stumbled upon a couple of lines where data is unnecessarily copied data during variable initialization (for which I opened #84379) and decided to scan the codebase with clang-tidy to see what it could find.

If this pull request conflicts with another one, priority should be given to the other pull request since this particular patch can be automatically generated with:

find . -name 'thirdparty' -prune -o -iname '*.cpp' -print -exec clang-tidy --checks=performance-unnecessary-copy-initialization -p $(pwd) --fix --fix-errors {} \;
git diff -U0  | grepdiff -E 'const.*&' --output-matching=hunk | sed -e 's/const \(.*\)& /const \1 \&/g' | git apply --cached --unidiff-zero
git reset platform/macos/export/export_plugin.cpp

And selectively choosing what to commit

[Riteo]

This comment is referring to a copy, with some confusion on what happens with reference copying.

I'd at least remove the comment but I'd investigate whether that's relevant anymore to the code it's in.

[Rubonnek]

I just checked and the comment is still relevant. There's a single EditorExportPlatformMacOS::_make_icon call from which p_icon comes from and there's a specific case where referencing p_icon is not needed. Properly patching this merits its own pull request. I'll undo this change from this PR.

[darksylinc]

Be careful, because Godot uses its own structures. This is not the STL.

For example this code:

String path = fields[0];

Does not actually copy. The String class is implemented using CoW (Copy on Write) which means a copy is almost free unless "path" is modified. If path is modified only then an actual copy occurs.

Changing it to:

const String &path = fields[0];

Just adds an extra indirection for no reason.

Something more controversial happens with:

Ref<Mesh> mesh = p_meshes[i];
// vs
const Ref<Mesh> &mesh = p_meshes[i];

The Ref<T> class is just a reference-counted object. Whether an extra indirection is better or incrementing and decrementing the reference count is better depends on profiling. It may also cause different behavior because consider the following:

void null_some_data()
{
   some_data = nullptr; // Decrease reference count. It may reach 0.
}


Ref<Mesh> mesh = some_data[i];
null_some_data();
mesh->do_something(); // valid

// vs

const Ref<Mesh> &mesh = some_data[i];
null_some_data();
mesh->do_something(); // ERROR! This is a nullptr.

[AThousandShips]

The data here are largely (with some exceptions) COW data types (Vector<T>, String, etc.), or reference types (Ref<T>), which means copying and creating references are largely equivalent

However for performance critical paths there is a small, but potentially important, difference:
COW has synchronization for copying, which can be performance negative, as any copying of a COW type invokes atomics

Additionally: The primary benefit of replacing with const references is that they can be optimized by the compiler, assuming other optimization assumptions hold

[Rubonnek]

What about set up some clang-tidy rules to check for CI instead of manully checking?

In the past we've opted not to mainly due to false positives and these usually happen when considering header guards, however there's other reasons as well.

One false positive I caught in this particular PR was that the tool suggested the following change:

index 2e0ca7b536..900f9b2714 100644
--- a/main/main.cpp
+++ b/main/main.cpp
@@ -1758,7 +1758,7 @@ Error Main::setup(const char *execpath, int argc, char *argv[], bool p_second_ph
 #endif
 
                String default_driver = driver_hints.get_slice(",", 0);
-               String default_driver_macos = default_driver;
+               const String &default_driver_macos = default_driver;
 #if defined(GLES3_ENABLED) && defined(EGL_STATIC) && defined(MACOS_ENABLED)
                default_driver_macos = "opengl3_angle"; // Default to ANGLE if it's built-in.
 #endif

You can get the above suggestion by scanning with:

clang-tidy --checks=performance-unnecessary-copy-initialization --fix main/main.cpp

At first glance it might seem fine, but upon closer inspection default_driver_macos should not be a const reference due to the header guards right below and the usage of the variable in the lines that follow. If we were to always check in CI with the tool, that patch will always show up assuming the surrounding code never gets refactored.

Another reason for not always checking the source with static analyzers is because we stumble upon Path Explosion. The case above is a small example of how clang-tidy didn't consider all possible cases. Now imagine if it did, and it did so across thousands of possible header values. The number of possible outcomes it has to handle increases quite quickly, meaning that the memory and CPU consumption to statically analyze a single compilation unit can vary wildly. This in turn means it's hard to gauge what resources are required to statically analyze any compilation unit in a CI pipeline. Which is another reason to avoid implementing it since there's a cost associated with it, and the return on investment does not seem to be worth the cost.

I think that covers most of our reasons. If we were to zoom out even more on this problem, we may end up talking about abstract topics such as the Halting Problem, Godel's Incompleteness, and other fundamental problems considered under specific contexts, at which point the answer to whether we should implement the CI check gets blurred by unnecessary details.

[jsjtxietian]

Very valid point, thanks!

[AThousandShips]

Second this, and adding that these kinds of static checks can easily confuse and mess with new contributors etc.

[AThousandShips]

Will take a look, but as suggested above many of these can be converted into simple uses, will recommend some tomorrow

[Rubonnek]

Will take a look, but as suggested above many of these can be converted into simple uses, will recommend some tomorrow

I should add there are many that I think add readability and perhaps should be kept, but I don't have a strong opinion about it. I don't mind simplifying further.

[AThousandShips]

Got around to it now, leaving it to your discression but most of these are direct improvements and several improve readability by not requiring you to check where the data is from (and are readable in what they represent)

[Rubonnek]

@AThousandShips the patches for most of your suggestions were included in #85071.

And thanks for the thorough review -- thanks to that I realized we could use a couple more const char * overloads to the Stringclass. I may add those to #84379 if I find the time later.

[AThousandShips]

These look good to me

[akien-mga]

Looks fine to me.

[akien-mga]

Thanks!