bames53 bames53 - 2 months ago 7
C++ Question

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

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

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);
}
Comments