3

I just started working with c++11 r-values. I read some tutorials, but I haven't found the answer.

What is the best way (the most efficient way) to set a class variable? Is below code correct or not? (let's assume std::string has defined move constructor and assignment operator).

class StringWrapper
{
private:
    std::string str_;

public:
    StringWrapper() : str_("") {}

    void setString1(std::string&& str) {
      str_ = std::move(str);
    }

    void setString2(const std::string& str) {
      str_ = std::move(str);
    }

    // other possibility?
};

int main() {
    std::string myText("text");

    StringWrapper x1, x2;

    x1.setString?("text"); // I guess here should be setString1
    x2.setString?(myText); // I guess here should be setString2
}

I know that compiler can optimize my code and/or I can use overload functions. I'd like to only know what is the best way.

marco
  • 111
  • 2
  • 8
  • 1
    You do not need to initiialize str_, therefore you do not need a user defined default constructor. –  Jun 22 '17 at 14:06
  • 1
    Have a look at how the standard library [declares such things](http://en.cppreference.com/w/cpp/string/basic_string/assign). It uses overloads. Btw. `std::move()` from a const reference doesn't make much sense. Use `str_ = str` in this case. – zett42 Jun 22 '17 at 14:11
  • 1
    `setString2` doesn't do what it looks like it's doing. `str_ = std::move(str);` where `str` is `const std::string&` will not actually preform a move assignment. – François Andrieux Jun 22 '17 at 14:24

4 Answers4

4

Compiler designers are clever folk. Use the crystal clear and therefore maintainable

void setString(const std::string& str) {
    str_ = str;
}

and let the compiler worry about optimisations. Pretty please, with sugar on top.

Better still, don't masquerade code as being encapsulated. If you intend to provide such a method, then why not simply make str_ public? (Unless you intend to make other adjustments to your object if the member changes.)

Finally, why don't you like the default constructor of std::string? Ditch str_("").

Bathsheba
  • 231,907
  • 34
  • 361
  • 483
  • In which way does OP _masquerade code as being encapsulated_? – zett42 Jun 22 '17 at 14:13
  • 2
    In providing a function to set the member without doing anything other than assigning it to the value passed in. – Bathsheba Jun 22 '17 at 14:14
  • Does not this do a double copy when rvalue is passed in? – Basilevs Jun 22 '17 at 14:23
  • Conceptually yes, but a good compiler will optimise it out, especially for `std::string`. – Bathsheba Jun 22 '17 at 14:24
  • _In providing a function to set the member without doing anything other than assigning it to the value passed in._ ... that's what the code is doing **now** but it might be changed in the **future** to do additional things. If you make `str_` public you won't be able to do so. – zett42 Jun 22 '17 at 15:57
  • @zett42: Absolutely. I believe I make that clear with "Unless you intend to make other adjustments to your object if the member changes.". – Bathsheba Jun 22 '17 at 15:57
  • To me this reads like you address present intentions only. It's not always known what the future brings ;-) – zett42 Jun 22 '17 at 16:01
  • @zett42: It's always a balancing act between future proofing and overdesign. In this particular case, you could always retrospectively mark the member `protected` or `private` and fix the compiler errors one by one. – Bathsheba Jun 23 '17 at 06:56
4

The version with rvalue reference would not normally bind to an lvalue (in your case, mytext), you would have to move it, and therefore construct the object twice, leaving you with a dangerous object. A const lvalue reference should be slower when constructing from an rvalue, because it would do the same thing again: construct -> move -> move construct.

The compiler could possibly optimize the overhead away though.

Your best bet would actually be:

void setString(std::string str) 
{
   str_ = std::move(str);
}

The compiler here is suprisingly guaranteed to deduce the type of the argument and call the copy constructor for lvalues and the move constructor for rvalues.

Update:

Chris Dew pointed out that constructing and move assigning a string is actually more expensive than copy constructing. I am now convinced that using a const& argument is the better option. :D

Adam Hunyadi
  • 1,890
  • 16
  • 32
  • 1
    Yes, this is indeed the way the trendies build "setters" nowdays. But if you know that, then the compiler will do too. I use the `const` ref way since I'm old fashioned. Have an upvote all the same. – Bathsheba Jun 22 '17 at 14:27
  • I really like the `const ref` way too, but one flaw it has is that it can create references of simple types such as `int`s even if copying them would be faster. I don't know how well this is optimized though. – Adam Hunyadi Jun 22 '17 at 14:36
  • 1
    The problem with this is that it does not re-use any existing capacity of `str_` in the case you pass an lvalue. If you call this multiple times with lvalues each time it will allocate a new string. If you have a `const std::string&` overload then it can re-use existing capacity and avoid allocations. – Chris Drew Jun 22 '17 at 14:45
  • @ChrheisDrew that is correct, but when using a `const ref` in your setter, you won't actually use the capacity of the argument, you will copy(!) it anyway. In the case of move constructing the compiler would know, that what is left behind is just garbage, so *theoretically* it should optimize the whole setter away (if it is not optimized, calling the destructor on the temporary could be awful for performance though). – Adam Hunyadi Jun 23 '17 at 07:04
  • @ChrisDrew okay, your cppcon video link persuaded me :D From now on, I'll use const refs as setters. – Adam Hunyadi Jun 23 '17 at 07:21
  • Yeah, just to be clear, it is not the capacity of the argument you would re-use it is the capacity of the target string. If you are passing an lvalue you have to make a copy of some sort but a direct copy assignment could be more efficient than copy constructing a new string and then a move assignment. – Chris Drew Jun 23 '17 at 07:52
4

Herb Sutter's advice on this is to start with the standard C++98 approach:

void setString(const std::string& str) {
  str_ = str;
}

And if you need to optimize for rvalues add an overload that takes an rvalue reference:

void setString(std::string&& str) noexcept {
  str_ = std::move(str);
}

Note that most implementations of std::string use the small string optimization so that if your strings are small a move is the same as a copy anyway and you wouldn't get any benefit.

It is tempting to use pass-by-value and then move (as in Adam Hunyadi's answer) to avoid having to write multiple overloads. But Herb pointed out that it does not re-use any existing capacity of str_. If you call it multiple times with lvalues it will allocate a new string each time. If you have a const std::string& overload then it can re-use existing capacity and avoid allocations.

If you are really clever you can use a templated setter that uses perfect forwarding but to get it completely correct is actually quite complicated.

Chris Drew
  • 14,926
  • 3
  • 34
  • 54
  • 1
    For me that's the pragmatic approach for everyday programming: Use the proven `const T&` by default, which is easy to write, has the least syntactic overhead and is easily understood even by less experienced fellow programmers. Add overload for rvalue reference only if profiling shows a need for it or if you have special use cases. – zett42 Jun 22 '17 at 17:57
3

You might probably use templatized setString and forwarding references:

class StringWrapper
{
private:
    std::string str_;

public:
    template<typename T>
    void setString(T&& str) {
        str_ = std::forward<T>(str);
    }
};
Edgar Rokjān
  • 17,245
  • 4
  • 40
  • 67
  • 2
    Herb Sutter [claims](https://www.youtube.com/watch?v=xnqTKD8uD64&t=1h14m58s) that to do this correctly you also have to add a complicated `enable_if` expression to constrain it to strings and a complicated conditional `noexcept` expression. – Chris Drew Jun 22 '17 at 15:18