[clang-tidy] Vote: Add efficiency checks

[j-stephan]

Voting will close on 09 September EOD (German time).

• Find structs that are inefficiently packed and/or aligned - Link
• Find inefficient calls to std::string::find - Link
• Find unnecessary copies of loop variables in range for loops - Link
• Find implicit copies in range for loops - Link
• Find inefficient usages of <algorithm> - Link
• Find inefficient std::string concatenations - Link. Strict mode enabled
• Find inefficient std::vector operations - Link
• Find useless calls to std::move - Link
• Find move constructors that initialize their base class or a member through a copy constructor - Link
• Find local variables that cannot automatically be moved upon return - Link
• Find move constructors / assignment operators without noexcept - Link
• Find types that could be trivially destructible but aren't - Link
• Find implicit type promotions when using <cmath>- Link
• Find unnecessary copy initializations - Link
• Find unnecessary copies that could be a const& - Link

Vote template

Check | Yes | No | Comment
------|-----|----|--------
Struct packing / alignment | X | X | X
`std::string::find` | X | X | X
Copies of loop vars in range for | X | X | X
Implicit copies in range for | X | X | X
Inefficient `<algorithm>` | X | X | X
Inefficient `std::string` concat | X | X | X
Inefficient `std::vector` ops | X | X | X
Useless `std::move` | X | X | X
copy ctors in move ctor | X | X | X
Unmovable local vars | X | X | X
Move ctors / assignments without `noexcept` | X | X | X
Types that should be trivially destructible | X | X | X
Type promotions in `<cmath>` | X | X | X
Unnecessary copy initializations | X | X | X
Unnecessary copies that could be `const&` | X | X | X

Vote

Check	Yes	No	Comment
Struct packing / alignment	1	0	-
std::string::find	1	0	-
Copies of loop vars in range for	1	0	-
Implicit copies in range for	1	0	-
Inefficient <algorithm>	1	0	-
Inefficient std::string concat	1	0	-
Inefficient std::vector ops	1	0	-
Useless std::move	1	0	-
copy ctors in move ctor	1	0	-
Unmovable local vars	1	0	-
Move ctors / assignments without noexcept	1	0	-
Types that should be trivially destructible	1	0	-
Type promotions in <cmath>	1	0	-
Unnecessary copy initializations	1	0	-
Unnecessary copies that could be const&	1	0	-

[sbastrakov]

Tbh most of those seem like mostly pointless "optimization" to me, that also mostly make the code harder to read and develop.

I feel very often readability greatly outweights the tiny (if any) benefits and in rare cases it matters a developer could do it manually. E.g. all those string things could be relevant if an app does that a lot inside hot loops, where minor optimizations may also matter, but that is not what we have. And so it would be just annoying for no gain. So I am against most of that, with few exceptions. This applies to most of my No votes, a few extra comments are in the table.

Check	Yes	No	Comment
Struct packing / alignment	2	0	-
std::string::find	1	1	I prefer consistency and not have a single character a special case
Copies of loop vars in range for	1	1	I am afraid that makes writing such a loop in generic context cumbersome
Implicit copies in range for	2	0	-
Inefficient	2	0	This somewhat makes sense as O-complexity may be wrong as a result
Inefficient std::string concat	1	1	The last example seems like an actively harmful change wrt readability
Inefficient std::vector ops	1	1	-
Useless std::move	2	0	Given that treats generic code fine
copy ctors in move ctor	2	0	Given that treats generic code fine
Unmovable local vars	1	1	In principle I would like to have it, but the current PIConGPU style with lots of const (which I strongly dislike, but that's what we have) would trigger it everywhere
Move ctors / assignments without noexcept	1	1	I think this is only useful for types used with STL and a useless rule for most of our code
Types that should be trivially destructible	2	0	-
Type promotions in	2	0	-
Unnecessary copy initializations	1	1	I think that's what a compiler should normally do, and a developer can assist when needed, but not to litter the whole code base with it
Unnecessary copies that could be const&	2	0	This one appears reasonable

[SimeonEhrig]

Check	Yes	No	Comment
Struct packing / alignment	3	0	-
std::string::find	2	1	-
Copies of loop vars in range for	2	1	I think, this will not affect use. But in the end, it is a easy change.
Implicit copies in range for	3	0	-
Inefficient <algorithm>	3	0	-
Inefficient std::string concat	1	2	I agree with Sergei. The rule is bad for the readability. If we are in a hoot loop, we should do it, but not for the complete code
Inefficient std::vector ops	2	1	-
Useless std::move	3	0	-
copy ctors in move ctor	3	0	-
Unmovable local vars	2	1	Jason Turner made a nice C++ weekly about this: [1]
Move ctors / assignments without noexcept	2	1	Jason Turner made a nice C++ weekly about this: [1]
Types that should be trivially destructible	3	0	-
Type promotions in <cmath>	3	0	-
Unnecessary copy initializations	2	1	-
Unnecessary copies that could be const&	3	0	-

[1] https://www.youtube.com/watch?v=dGCxMmGvocE

[bernhardmgruber]

Check	Yes	No	Comment
Struct packing / alignment	3	1	Disabled in LLAMA because it is incredibly noisy. Please try it first in alpaka. Or make it project specific.
std::string::find	3	1	-
Copies of loop vars in range for	3	1	-
Implicit copies in range for	4	0	-
Inefficient <algorithm>	4	0	-
Inefficient std::string concat	2	2	+= works in most cases and does not hinder readability
Inefficient std::vector ops	3	1	-
Useless std::move	4	0	-
copy ctors in move ctor	4	0	-
Unmovable local vars	2	1	I abstrain, I like to keep my local vars const, but I am curious how the check turns out
Move ctors / assignments without noexcept	3	1	-
Types that should be trivially destructible	4	0	-
Type promotions in <cmath>	4	0	I am very curious if this will flag the entire alpaka cuda backend :)
Unnecessary copy initializations	3	1	-
Unnecessary copies that could be const&	4	0	-

[j-stephan]

Or make it project specific.

Well, it's not like these rules are coming to us from Mount Sinai. I think projects should always be allowed to change individual rules (minus the style rules) if it makes more sense.

[j-stephan]

@psychocoderHPC We need a tie breaker, please vote :-)

[psychocoderHPC]

Check	Yes	No	Comment
Struct packing / alignment	3	2	I trust @bernhardmgruber that it is bad for llama, I am also not sure which side effects this rule can have
std::string::find	4	1	-
Copies of loop vars in range for	4	1	-
Implicit copies in range for	5	0	-
Inefficient	5	0	-
Inefficient std::string concat	2	3	+ is much shorter than append therefore I am against this rule
Useless std::move	4	1	I am not sure which side effects this rule will have in templated code where you do not know if the data moved is trivially copyable.

I voted only for few options and solved the issue which 2:2 for

[bernhardmgruber]

I trust @bernhardmgruber that it is bad for llama, I am also not sure which side effects this rule can have

It is not bad, but it is super noisy. For example it flags all type traits, because they occupy 1 byte but have an alignment of 0 :) But just try yourself for alpaka! I think the checks makes sense for small programs and maybe when targeting embedded systems.