Add all the performance-* clang-tidy checks

[lucafedeli88]

This PR adds all the performance-* checks to clang-tidy CI test, namely:

• performance-faster-string-find
• performance-for-range-copy
• performance-implicit-conversion-in-loop
• performance-inefficient-algorithm
• performance-inefficient-string-concatenation
• performance-inefficient-vector-operation
• performance-move-const-arg
• performance-move-constructor-init
• performance-no-automatic-move
• performance-no-int-to-ptr
• performance-noexcept-move-constructor
• performance-trivially-destructible
• performance-type-promotion-in-math-fn
• performance-unnecessary-copy-initialization
• performance-unnecessary-value-param

[lucafedeli88]

I see that a test case is failing. I will investigate

[lucafedeli88]

I see that a test case is failing. I will investigate

The bug should be fixed!

[franzpoeschel]

Currently looking through this. The checks are very useful, thank you for adding this!

It seems that in some places the suggested fixes by clang-tidy started an avalanche of recursive fixes and I'm trying to undo those. (Basic pattern: A std::move() was forgotten or wrongly applied, so clang-tidy suggests to use a const & argument and that change then propagates through the entire code base)

Feel free to ping either Axel or me when you need to have the CI started

[franzpoeschel]

I've taken the freedom to push a commit on this branch that reverts some of the changes and tries to fix the warnings in another way. Most of these were "pass by value" turned into "pass by reference" because of:

• std::move was forgotten or wrongly applied. Fix that instead of changing the function signatures.
• API consistency: Some parameters are needed only in some configurations. clang-tidy then thinks that an unused parameter should be turned to a reference. Depending on the #ifdef logic, several implementations may be chosen. Here, we should optimize for the implementations that actually use the parameters since the others are often just there to satisfy the linker.
• Planned changes: A parameter is not yet used today, but will be in future. clang-tidy again thinks that unused parameters should be references.

The one thing that is a bit dangerous about clang-tidy imo is that it is very eager to change the API and ABI because of an implementation detail. This is mostly fine since those cases can mostly be handled case-by-case, but one should be aware of it.

One method that seems to successfully trick clang-tidy is to define something like template<typename T> void forget(T){} and passing unused parameters off into that template. I'll need to add a second commit that puts this in its own header, but not today.

Let's first see what the CI now thinks of this.

EDIT: I seem to have broken some things. Will investigate next week.

[franzpoeschel]

Hey @lucafedeli88 The CI is now passing. If you are fine with the changes I did (only in the code, I did not touch the checks), then I would merge this.

EDIT: I forgot one thing, CI needs to run again

[franzpoeschel]

Merging this now, thank you for adding this! Since many files are affected, this is likely to cause merge conflicts. But since each single change is mostly rather simple, the single merge conflicts should not be hard to resolve. In doubt, current changes can be replaced by incoming changes, and then clang-tidy suggestions can be reapplied.