I have this MCVE:
#include <stdio.h>
#include <string>
#include <utility>
#include <vector>
enum class someEnum { first, second, none, };
template<typename T> class fooClass
{
public:
fooClass( std::vector<std::pair<const char *, T>> _classObject )
: classObject( _classObject ) {};
auto findMember( const std::string & memberName, T defaultReturn ) const -> T;
private:
const std::vector<std::pair<const char *, T>> classObject;
};
template<typename T> auto fooClass<T>::findMember( const std::string & memberName,
T defaultReturn
) const -> T
{
for ( auto const & it : classObject ) {
if ( 0 == memberName.compare( it.first ) ) {
return it.second;
}
}
return defaultReturn;
}
int main() {
fooClass<someEnum> somePairs( {
{ "text1", someEnum::first }, { "text2", someEnum::second }, });
printf( "Value=%d\n", somePairs.findMember( "text2", someEnum::none ) );
}
I ran Cppcheck and it told me this:
Summary: Function parameter '_classObject' should be passed by reference. Message: Parameter '_classObject' is passed by value. It could be passed as a (const) reference which is usually faster and recommended in C++.
for this line:
fooClass( std::vector<std::pair<const char *, T>> _classObject )
: classObject( _classObject ) {};
Ok, I changed then the declaration of classObject like this:
| Added the & operand
V
const std::vector<std::pair<const char *, T>> & classObject;
gcc with the highest warning level accepts this change but when I run the program it crashes. The reason for the crash is that it.first is null.
What do I have to change that a reference is used and the program works?
The recommendation is for your constructor argument, not for your class member.
By making your data member a reference you introduce an implicit requirement that the referred object must have a lifetime at least as long as the instance of fooClass. But you are making it refer to _classObject, which is a function argument taken by value, a local copy of the vector passed to the constructor. This local copy's lifetime is limited the to duration of the constructor, which is to say much shorter than the lifetime of the instance being constructed. Trying to access the referred object after this point is undefined behavior.
According to the recommendation, your constructor should look like this :
fooClass(const std::vector<std::pair<const char *, T>> & _classObject)
By making the argument take a const reference you should avoid the creation of an unnecessary copy of the vector. However, since std::vector supports move semantics, the gains in performance will likely not be as significant in this case. It would be more noticeable if the vector was fully copied.
Since it seems like you want fooClass to take ownership of the contents of the given vector (or of a copy of it) it would be wise to also provide a Rvalue reference overload for your constructor. Otherwise, in cases like the example you provided, a copy of the data will be made when the reference is assigned to classObject which may not be necessary.
A common solution to avoid having multiple overloads is to only provide one overload that takes the object by value and to then move that copy into the class' data member. It's less optimal in that an instance may be needlessly move constructed, but it avoids code duplication. Rather or not this approach is right for you is up to you.
Finally, consider rather or not classObject should be const. Doing so prevents your class from being assignable and movable. But what are the benefits?