-1

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?

  • 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. – Deduplicator Dec 20 '18 at 13:36
  • if you use std::string_view do you still use std::move ? – Sandro4912 Dec 20 '18 at 15:16
  • You use std::string&& and move, as well as std::string_view and copy. – Deduplicator Dec 20 '18 at 16:27
  • i fixed the second example i pasted the wrong code. sorry for that – Sandro4912 Dec 21 '18 at 8:48
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.

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");
        }
    }
};
  • I thought in general getters should return by value not by const reference. – Sandro4912 Dec 20 '18 at 14:13
  • 1
    @Sandro4912 It doesn't matter for types that can only be copied, but for types that benefit from moving, no, you don't – Caleth Dec 20 '18 at 14:19
  • @Sandro4912 and I would just have public: std::string value; in this case – Caleth Dec 20 '18 at 14:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.