Allow aggregate initialisation of components?

[radman0x]

entityx/entityx/Entity.h

Line 648 in 6389b1f

::new(pool->get(id.index())) C(std::forward<Args>(args) ...);

Seems like the line I've tagged could be easily changed to use brace initialisation rather than direct initialisation. I'm wondering if this has been considered previously and any reason for the choice to avoid it.

Brace initialisation would allow for aggregate initialisation meaning that you don't need to manually create a constructor for each component. Given that components should frequently qualify as aggregates this would seem to be a nice improvement by removing the need to create constructors unnecessarily.

Thoughts?

[alecthomas]

Yes this has been considered before, but I believe it would break API compatibility.

[alecthomas]

See #87.

[alecthomas]

I wonder if it's possible to achieve backwards compatibility with SFINAE...

[radman0x]

Hmmm I suspect trying to SFINAE to the correct types would be a direct clash with the use of variadic arguments, but that's just my initial feel.

In general though the narrowing conversion issues are pretty easy to get around. They only really occur when using literals or ambiguous temporaries. This can be resolved by ensuring the right types are passed in e.g. int(5) rather than just 5 or std::string("oeau") instead of "oeau". Still would be a major compatibility issue for existing code bases. For me though I've made the change locally as removing the need to write endless redundant constructors comes with almost no cost. I think the change is a good one in general for any new project using EntityX.

[FrankStain]

@alecthomas , @radman0x
Hi there! I just leave my 5 cent here.

We may implement the selection of construction type using the code like that:

template< typename TComponent, typename... TArguments >
inline typename std::enable_if<IS_AGGREGATE<TComponent>, TComponent*>::type Construct(void* component_memory, TArguments&&... arguments)
{
   // Brace-initialization / aggregate construction.
   return new( component_memory ) TComponent{ std::forward<TArguments>( arguments )... };
}

template< typename TComponent, typename... TArguments >
inline typename std::enable_if<!IS_AGGREGATE<TComponent>, TComponent*>::type Construct(void* component_memory, TArguments&&... arguments)
{
   // Narrowing construction.
   return new( component_memory ) TComponent( std::forward<TArguments>( arguments )... );
}

The question is inside of IS_AGGREGATE term...

Currently, using the C++17 standard, we can operate with std::is_aggregate trait.
Here is the doc: https://en.cppreference.com/w/cpp/types/is_aggregate
But std::is_aggregate will break the compatibility with earlier C++ standards.

For earlier C++ standards we can use the std::is_constructible trait, which is implemented since C++11.
We can think on it this way. When we declare the aggregate type, the std::is_constructible trait will always be false.
On the other side, when we declare the conversion constructor (or even manual aggregate constructor) of any kind, we potentially hope on narrowing conversion.
std::is_constructible trait will be true in such situation.
As the result, the false check will enable the function with brace-initialization and the true one will enable the function with narrowing construction.

Here is the link to test code, which uses the std::is_constructible trait.

BTW, @alecthomas , what C++ standard is current for EntityX? Is it still C++11?

[alecthomas]

Thanks @FrankStain .

You know what, let me tag a stable version, then if you feel like sending a PR with your proposed change @radman0x, I'll merge it. Better to keep adapting it then stay static and annoying.

[alecthomas]

Okay, tagged 1.3.0.

[radman0x]

@alecthomas
I'll look at sending my raw changes through in a PR, I've also found a second location where brace initialisation needed to be added. Changes are trivial, literally just changing the parens to braces.

@FrankStain
Thanks for pointing out is_aggregate and is_constructible but I'm not sure that it will be able to remove the narrowing conversion issues. Brace initialisation is uniform and can be used with aggregates or to call defined constructors transparently. I can't see what adding in traits to specialise separately can add as the issue is the types of the parameters being passed to the constructor (or aggregate init) rather than the syntax used to trigger construction. So the issue is getting the types of the Construct arguments to match exactly what the aggregate expects... Maybe I've missed something, feel free to elaborate if that's the case, I'm still mulling over if there's any other way.

As an aside I think usage of is_aggregate is out of reach for the moment as I don't think VS supports it and from what I can gather g++ needs 7.1 or above :(

[radman0x]

Ahhh I see, haha it's always right after you finish a response :P Specialising for aggregates would ease the backwards compatibility issues as any current code won't use brace initialisation as this is reserved only for aggregates.

[alecthomas]

Don't worry about backwards compatibility.

[kim366]

This is why I am using assign_from_copy more than I do assign, but this is quite suboptimal, since sometimes I'm copying entire STL containers. Something like assign_from_move or massign would be nice.

[kim366]

Huh, interesting that that PR was all it took. I'll update later and make my code a lot shorter :)

[kim366]

Why was it reverted? Couldn’t find any explanation