Using std::move in Constructor with value error handling

Question

Im confuesd about the usage of std::move in class constructors.

I want to pass a value to a class and check in the constructor if a valid value is passed. If not an exception is thrown.

Before i used to pass the value by const reference:

#include <string>
class Foo {
public:
 Foo(const std::string& value)
 :m_value{ value }
 {
 if (value == "Foo") {
 throw std::invalid_argument("Invalid");
 }
 }
private:
 std::string m_value;
};

Now i read in modern c++ it is better to pass by value and move the passed value into the class variable:

#include <string>
class Foo {
public:
 Foo(std::string value)
 :m_value{ std::move (value) }
 {
 // Now m_value must be used because value is empty if reference
 //if(value == "Foo") -> Desaster value is moved away = empty
 if (m_value == "Foo") {
 throw std::invalid_argument("Invalid");
 }
 }
private:
 std::string m_value;
};

So far so good i can just refer to the class variable to do my error checking. Now i started to modernize a codebass which has a class hierachy were an value is already defined in the base class. The child class basically performs additional checks on the variable and throws.

Here a simplified example:

#include <string>
class Bar {
public:
 Bar(std::string value)
 :m_value{std::move(value))}
 {
 // value is "empty" here because its moved into m_value
 if (m_value == "Foo") {
 throw std::invalid_argument("Invalid");
 }
 }
 std::string get_value() const { return m_value; }
 void set_value(const std::string& value)
 {
 m_value = value;
 }
private:
 std::string m_value;
};
// Version 1
// We have to ask several times the base class to get the compare value
class Foo : public Bar {
public:
 Foo(std::string value)
 :Bar{ std::move(value) }
 {
 if (get_value() == "BAR" || 
 get_value() == "Bar" || // perform several checks here
 get_value() == "bar") {
 throw std::invalid_argument("Invalid");
 }
 }
};
// Version 2
// We need to make a copy to check. Doesn't this ruin the purpose of 
// std::move by saving a copy??
class Foo : public Bar {
public:
 Foo(std::string value)
 :Bar{ std::move(value) }
 {
 auto check_value = get_value();
 if (check_value == "BAR" ||
 check_value == "Bar" || // perform several checks here
 check_value == "bar") {
 throw std::invalid_argument("Invalid");
 }
 }
};
// Version 3
// doesn't work except we provide in the base class a default constructor
class Foo : public Bar {
public:
 Foo(std::string value)
 //Error here we need to provide a default constructor
 {
 if (value == "BAR" ||
 value == "Bar" || // perform several checks here
 value == "bar") {
 throw std::invalid_argument("Invalid");
 }
 set_value(std::move(value));
 }
};

So my question is, which approach is the best. Or how can this be handled better?

• Deduplicator: I don't see any moving going on in your second example... also, you should prefer std::string_view-parameters over std::string const&. std::string or std::string&& is only superior when you have an expiring value anyway.
• Sandro4912: if you use std::string_view do you still use std::move ?
• Deduplicator: You use std::string&& and move, as well as std::string_view and copy.
• Sandro4912: i fixed the second example i pasted the wrong code. sorry for that

Answer 1

Just do the error-checking first:

Bar(std::string value)
: m_value{m_value == "Foo" ? throw std::invalid_argument("Invalid") : std::move(value)}
{}

Respectively, if the error-checking is more complicated, consider putting it in its own function:

void check_args(std::string const& s) {
 if (get_value() == "BAR" // perform several checks here
 || get_value() == "Bar"
 || get_value() == "bar")
 throw std::invalid_argument("Invalid");
}
Foo(std::string value)
: Bar{ (check_args(value), std::move(value)) }
{
}

Also consider two constructors, accepting respectively a std::string&& (if you already have a temporary) or a std::string_view (if you don't).
Delegating to a common implementation internally is pretty trivial.

Answer 2

The problem you are encountering is that Bar::get_value() eagerly copies. The advice is that parameters should be values, but results might still sensibly be const &.

class Bar {
public:
 Bar(std::string value)
 :m_value{std::move(value))}
 {
 // value is "empty" here because its moved into m_value
 if (m_value == "Foo") {
 throw std::invalid_argument("Invalid");
 }
 }
 const std::string & get_value() const { return m_value; }
 void set_value(std::string value)
 {
 m_value = std::move(value);
 }
private:
 std::string m_value;
};
// We have to ask several times the base class to get the reference, and the compiler is able notice we don't change it in between
class Foo : public Bar {
public:
 Foo(std::string value)
 :Bar{ std::move(value) }
 {
 // value is "empty" here because its moved into Bar
 if (get_value() == "BAR" || 
 get_value() == "Bar" || // perform several checks here
 get_value() == "bar") {
 throw std::invalid_argument("Invalid");
 }
 }
};

• Sandro4912: I thought in general getters should return by value not by const reference.
• Caleth: @Sandro4912 It doesn't matter for types that can only be copied, but for types that benefit from moving, no, you don't
• Caleth: @Sandro4912 and I would just have public: std::string value; in this case