What's the correct `enable_if` constraint on perfect forwarding setter?

Question

Herb Sutter's Back to the Basics! Essentials of Modern C++ presentation at CppCon discussed different options for passing parameters and compared their performance vs. ease of writing/teaching. The 'advanced' option (providing the best performance in all the cases tested, but too difficult for most developers to write) was perfect forwarding, with the example given (PDF, pg. 28):

class employee {
    std::string name_;

public:
    template <class String,
              class = std::enable_if_t<!std::is_same<std::decay_t<String>,
                                                     std::string>::value>>
    void set_name(String &&name) noexcept(
      std::is_nothrow_assignable<std::string &, String>::value) {
        name_ = std::forward<String>(name);
    }
};

The example uses a template function with forwarding reference, with the template parameter String constrained using enable_if. However the constraint appears to be incorrect: It seems to be saying this method may be used only if the String type is not a std::string, which makes no sense. That would mean that this std::string member can be set using anything but an std::string value.

using namespace std::string_literals;

employee e;
e.set_name("Bob"s); // error

One explanation I considered was that there's a simple typo and the constraint was intended to be std::is_same<std::decay_t<String>, std::string>::value instead of !std::is_same<std::decay_t<String>, std::string>::value. However that would imply that the setter doesn't work with, e.g., const char * and it obviously was intended to work with this type given that that's one of the cases tested in the presentation.

It seems to me that the correct constraint is more like:

template <class String,
          class = std::enable_if_t<std::is_assignable<decltype((name_)),
                                                      String>::value>>
void set_name(String &&name) noexcept(
  std::is_nothrow_assignable<decltype((name_)), String>::value) {
    name_ = std::forward<String>(name);
}

allowing anything that can be assigned to the member to be used with the setter.

Have I got the right constraint? Are there other improvements that can be made? Is there any explanation for the original constraint, perhaps it was taken out of context?

---

Also I wonder if the complicated, 'unteachable' parts of this declaration are really that beneficial. Since we're not using overloading we can simply rely on normal template instantiation:

template <class String>
void set_name(String &&name) noexcept(
  std::is_nothrow_assignable<decltype((name_)), String>::value) {
    name_ = std::forward<String>(name);
}

And of course there's some debate over whether noexcept really matters, some saying not to worry too much about it except for move/swap primitives:

template <class String>
void set_name(String &&name) {
    name_ = std::forward<String>(name);
}

Maybe with concepts it wouldn't be unreasonably difficult to constrain the template, simply for improved error messages.

template <class String>
  requires std::is_assignable<decltype((name_)), String>::value
void set_name(String &&name) {
    name_ = std::forward<String>(name);
}

This would still have the drawbacks that it can't be virtual and that it must be in a header (though hopefully modules will eventually render that moot), but this seems fairly teachable.

Answer 1

I think what you have is probably right, but in the interest of not writing an "answer" that is simply "I agree", I will propose this instead that will check assignment based on the correct types - whether it's an lval, rval, const, whatever:

template <class String>
auto set_name(String&& name) 
-> decltype(name_ = std::forward<String>(name), void()) {
    name_ = std::forward<String>(name);
}

Answer 2

One explanation I considered was that there's a simple typo and the constraint was intended to be std::is_same<std::decay_t<String>, std::string>::value instead of !std::is_same<std::decay_t<String>, std::string>::value.

Yes, the right constraint that was presented on the screen was is_same, not !is_same. Looks like there is a typo on your slide.

---

However that would imply that the setter doesn't work with, e.g., const char *

Yes, and I believe this was done on purpose. When a string literal like "foo" is passed to a function accepting a universal reference, then the deduced type is not a pointer (since arrays decay to pointers only when caught in template parameter by value), rather, it is a const char(&)[N]. That said, every call to set_name with string literal of different length would instantiate a new set_name specialization, like:

void set_name(const char (&name)[4]); // set_name("foo");
void set_name(const char (&name)[5]); // set_name("foof");
void set_name(const char (&name)[7]); // set_name("foofoo");

The constraint is supposed to define the universal reference so that it accepts and deduces only either std::string types for rvalue arguments, or cv-std::string& for lvalue arguments (that's why it is std::decayed before comparing to std::string in that std::is_same condition).

---

it obviously was intended to work with this type given that that's one of the cases tested in the presentation.

I think the tested version (4) was not constrained (note it was named String&&+perfect forwarding), so it could be as simple as:

template <typename String>
void set_name(String&& name)
{
    _name = std::forward<String>(name);
}

so that when a string literal is passed, it does not construct the std::string instance prior to function call just like the non-templated versions would do (unnecessarily allocating memory in callee's code just to build a std::string temporary that would be eventually moved to possibly preallocated destination like _name):

void set_name(const std::string& name);
void set_name(std::string&& name);

---

It seems to me that the correct constraint is more like: std::enable_if_t<std::is_assignable<decltype((name_)), String>::value>>

No, as I wrote, I don't think the intent was to constrain the set_name to accept types assignable to std::string. Once again - that original std::enable_if is there to have a single implementation of set_name function taking a universal reference accepting only std::string's rvalues and lvalues (and nothing more although this is a template). In your version of std::enable_if passing anything non-assignable to std::string would yield an error no matter if the constaint is or not when tried to do so. Note that eventually we might be just moving that name argument to _name if it's non-const rvalue reference, so checking assignability is pointless except from situations when we are not using a SFINAE to exclude that function from overload resolution in favor of other overload.

---

What's the correct enable_if constraint on perfect forwarding setter?

template <class String,
          class = std::enable_if_t<std::is_same<std::decay_t<String>,
                                                std::string>::value>>

or no constraint at all if it won't cause any performance hit (just like passing string literals).

Answer 3

I tried to fit these thoughts in a comment, but they would not fit. I presume to write this as I am mentioned both in the comments above, and in Herb's great talk.

Sorry to be late to the party. I have reviewed my notes and indeed I am guilty of recommending to Herb the original constraint for option 4 minus the errant !. Nobody is perfect, as my wife will assuredly confirm, least of all me. :-)

Reminder: Herb's point (which I agree with) is start with the simple C++98/03 advice

set_name(const string& name);

and move from there only as needed. Option #4 is quite a bit of movement. If we are considering option #4, we are down to counting loads, stores and allocations in a critical part of the application. We need to assign name to name_ just as fast as possible. If we are here, code readability is far less important than performance, though correctness is still king.

Providing no constraint at all (for option #4) I believe to be slightly incorrect. If some other code attempted to constrain itself with whether or not it could call employee::set_name, it could get the wrong answer if set_name is not constrained at all:

template <class String>
auto foo(employee& e, String&& name) 
-> decltype(e.set_name(std::forward<String>(name)), void()) {
    e.set_name(std::forward<String>(name));
    // ...

If set_name is unconstrained and String deduces to some completely unrelated type X, the above constraint on foo improperly includes this instantiation of foo in the overload set. And correctness is still king...

What if we want to assign a single character to name_? Say A. Should it be allowed? Should it be wicked fast?

e.set_name('A');

Well, why not?! std::string has just such an assignment operator:

basic_string& operator=(value_type c);

But note that there is no corresponding constructor:

basic_string(value_type c);  // This does not exist

Therefore is_convertible<char, string>{} is false, but is_assignable<string, char>{} is true.

It is not a logic error to try to set the name of a string with a char (unless you want to add documentation to employee that says so). So even though the original C++98/03 implementation did not allow the syntax:

e.set_name('A');

It did allow the same logical operation in a less efficient manner:

e.set_name(std::string(1, 'A'));

And we are dealing with option #4 because we are desperate to optimize this thing to the greatest degree possible.

For these reasons I think is_assignable is the best trait to constrain this function. And stylistically I find Barry's technique for spelling this constraint quite acceptable. Therefore this is where my vote goes.

Also note that employee and std::string here are just examples in Herb's talk. They are stand-ins for the types you deal with in your code. This advice is intended to generalize to the code that you have to deal with.