Variadic template issue

Question

I have the following code I'm trying to adapt for my own purposes. This is a learning exercise for me to attempt an update of my C++ skills. This was originally written with Clang 3.1 in mind as far as I can tell.

I've tried compiling with Clang versions from 3.1 to 4.0 and GCC 4.9 to 7.1 with very similar results.

These are the error messages from GCC 5.1

Error 1: In instantiation of 'constexpr Struct<Fields>::Struct(T&& ...) [with T = {Struct<foo<int>, bar<double> >&}; Fields = {foo<int>, bar<double>}]':
<source>:46:12:   required from here

Error 2: <source>:28:61: error: mismatched argument pack lengths while expanding 'Fields'
 constexpr Struct(T &&...x) : Fields{static_cast<T&&>(x)}... {}

Please ELI5 if you have the patience :P

You can see this in godbolts compiler doohickey here: https://godbolt.org/g/2dRPXf

EDIT:

Given the answers by @Passer-By and @Daniel-Jour, I wonder if Struct(Struct const &) = default; is even necessary. Will removing this constructor from Struct have effects of which I am not aware or do no foresee (I am no C++ Swami!)?

Is the constructor constexpr Struct(T &&...x) : Fields{static_cast<T&&>(x)}... {} a good stand-in for what would otherwise be generated by Struct(Struct const &) = default;?

I'm feeling pretty ambiguous about either proposed solution so far.

End EDIT

// From http://duriansoftware.com/joe/Self-aware-struct-like-types-in-C++11.html
// edited a bit to stop the compiler whining about comments in multiline macros
#include <type_traits>
#include <utility>

#define SELFAWARE_IDENTIFIER(NAME) \
    template<typename T> \
    struct NAME { \
        T NAME; /* field name */ \
        constexpr static char const *name() { return #NAME; } /* field type */ \
        using type = T; /* field value generic accessor */ \
        T &value() & { return this->NAME; } \
        T const &value() const & { return this->NAME; } \
        T &&value() && { return this->NAME; } \
    };

template<typename...Fields>
struct Struct : Fields... {
    // A convenience alias for subclasses
    using struct_type = Struct;

    // Preserve default constructors
    Struct() = default;
    Struct(Struct const &) = default;

    // Forwarding elementwise constructor
    template<typename...T>
    constexpr Struct(T &&...x) : Fields{static_cast<T&&>(x)}... {} // Error 2 here
};

SELFAWARE_IDENTIFIER(foo)
SELFAWARE_IDENTIFIER(bar)
// Aliasing a Struct instance
using FooBar = Struct<foo<int>, bar<double> >;
// Inheriting a Struct instance (requires inheriting constructors)
struct FooBar2 : Struct<foo<int>, bar<double>> { using struct_type::struct_type; };

static_assert(std::is_trivial<FooBar>::value, "should be trivial");
static_assert(FooBar{2, 3.0}.foo + FooBar{2, 4.0}.foo == 4, "2 + 2 == 4");


FooBar frob(int x) {
    FooBar f = {x, 0.0};
    f.foo += 1;
    f.bar += 1.0;
    return f; // Error 1 here
}

Answer 1

You've fallen victim of what I know as "too perfect forwarding".

To debug this, first look closely at the error message:

instantiation of constexpr Struct<Fields>::Struct(T&& ...) [with T = {Struct<foo<int>, bar<double> >&}; Fields = {foo<int>, bar<double>}]:

This tells us that this line of code

return f;

does not as expected call the copy or move constructor, but rather the perfect forwarding constructor.

Looking at what this constructor does, it's clear that this constructor is not capable of copying or moving from a Struct. Its intended​ use case is to construct each of the fields from one of the arguments. But the error message shows that there is only a single argument of type Struct<foo<int>, bar<double> >&. Because the expansion of the arguments also expands Fields (of which there are two) you get that second error:

[..] error: mismatched argument pack lengths [..]

But why does it take the perfect forwarding constructor instead of the also available copy constructor? Because the forwarding constructor is able to provide a better candidate (an exact match actually) than the copy constructor (whose signature is Struct(Struct const &)): Struct(Struct & &&), which according to the reference combination rules is Struct(Struct &). And that's exactly what's needed to use f in return f;: after all f is a non const lvalue.

One possibility to fix this is to provide another (copy) constructor with the exact signature:

Struct(Struct & s)
  : Struct(static_cast<Struct const &>(s))
{}

But if you also add in volatile you need to write a total of six constructors to have all cases covered. Not nice.

The better solution is to exclude the perfect forwarding constructor from overload resolution with SFINAE:

template<typename T>
using decay_t = typename decay<T>::type;

template<
  typename...T,
  std::enable_if<
    (sizeof...(T) == sizeof...(Fields))
    && not_self_helper<Struct, std::tuple<decay_t<T>...>>::value
    >::type * = nullptr
  >
constexpr Struct(T &&...x)
  : Fields{static_cast<T&&>(x)}... {}

not_self_helper checks whether a single parameter passed to a structure with a single field is of the structures own type:

template<typename, typename>
struct not_self_helper : std::true_type {};

template<typename Self>
struct not_self_helper<Self, std::tuple<Self>> : std::false_type {};

This fixes your main issue: The forwarding constructor is semantically just wrong. It does not take an arbitrary number of parameters, but needs exactly the same number of parameters as the structure has fields. Further, none of the fields should be constructed from the containing structure itself (recursive membership means infinite structure size, after all). I shortened that test to only check when there's a single argument, though. Strictly speaking, this is semantically wrong, but in practice it covers the most "dangerous" case: When your forwarding constructor is wrongfully selected for copy/move construction.

Answer 2

The problem consists of two parts

Overload resolution

For a variadic constructor with forwarding references

template<typename... Args>
Struct(Args&&...);

Given any lvalue arguments, Args will be deduced as lvalue references, and rvalue arguments rvalue references.

For another constructor to be called, the variadic constrcutor must not have a better conversion sequence than the constructor.

Your code includes only one other constructor that takes one parameter, the copy constructor

Struct(const Struct&);

A lvalue const Struct will bind with the copy constructor's const Struct& while a lvalue or rvalue Struct will bind with the variadic constructor's Struct& or Struct&& respectively.

Value category of returned type

A glvalue expression referring to an automatic duration variable declared in the function in a return statement is considered a xvalue, and would therefore bind to Struct&& first. When that fails, the expression will be considered a lvalue and proceed to bind to Struct& or const Struct&.

In this case, Struct&& succeeds with the variadic constructor, but results in an error.

If a move constructor is provided, the move constructor will be selected after overload resolution discards the variadic constructor.

Responding to edit

Removing your user-provided copy constructor will allow for an implicitly declared move constructor, which will make the code compile, following the reasons above.

The variadic constructor is not a good stand in for either constructors, aside from being semantically wrong, it takes arbitrary arguments but requires a fixed (in this case 2) amount of arguments to initialize the fields. If it were to be used as a copy constructor, you will get the same compile error: mismatched argument pack lengths while expanding 'Fields'

As was mentioned in Daniel Jour's answer, you should probably put some SFINAE in the variadic constructor to alleviate some pain.