Entt: movable but non copyable components

[markand]

Hello,

According to the code of assign, since it uses perfect forwarding, it should move my non-copyable objects instead of trying to copy it.

To explain:

I'd like to create a component that stores a polymorphism set of actions (feel free to advise a new system if you have any idea). The purpose of this is to create a sequence of actions to perform on that entity, e.g.:

• move to x, y during 5 seconds
• wait here
• move somewhere else
• rotate

I'm actually using this sequence of polymorphism object as a separate code into the main logic, but I wanted to merge them in the entity so that each entity can have different set of actions.

Here's the code

#include <memory>
#include <vector>

#include <entt/entt.hpp>

struct noncopyable {
        // just for testing...
        std::vector<std::unique_ptr<int>> actions;
};

int main()
{
        entt::registry reg;
        entt::entity et = reg.create();

        noncopyable sequence;

        reg.assign<noncopyable>(et, std::move(sequence));
}

In the real code, the noncopyable structure holds a vector of unique_ptr of action (described above). But now trying to assign a component and trying to move still tries to copy it at some point:

C:/msys64/mingw64/include/c++/9.2.0/bits/stl_construct.h: In instantiation of 'void std::_Construct(_T1*, _Args&& ...) [with _T1 = std::unique_ptr<int>; _Args = {const std::unique_ptr<int, std::default_delete<int> >&}]':
C:/msys64/mingw64/include/c++/9.2.0/bits/stl_uninitialized.h:83:18:   required from 'static _ForwardIterator std::__uninitialized_copy<_TrivialValueTypes>::__uninit_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<int>*, std::vector<std::unique_ptr<int> > >; _ForwardIterator = std::unique_ptr<int>*; bool _TrivialValueTypes = false]'
C:/msys64/mingw64/include/c++/9.2.0/bits/stl_uninitialized.h:134:15:   required from '_ForwardIterator std::uninitialized_copy(_InputIterator, _InputIterator, _ForwardIterator) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<int>*, std::vector<std::unique_ptr<int> > >; _ForwardIterator = std::unique_ptr<int>*]'
C:/msys64/mingw64/include/c++/9.2.0/bits/stl_uninitialized.h:289:37:   required from '_ForwardIterator std::__uninitialized_copy_a(_InputIterator, _InputIterator, _ForwardIterator, std::allocator<_Tp>&) [with _InputIterator = __gnu_cxx::__normal_iterator<const std::unique_ptr<int>*, std::vector<std::unique_ptr<int> > >; _ForwardIterator = std::unique_ptr<int>*; _Tp = std::unique_ptr<int>]'
C:/msys64/mingw64/include/c++/9.2.0/bits/stl_vector.h:555:31:   required from 'std::vector<_Tp, _Alloc>::vector(const std::vector<_Tp, _Alloc>&) [with _Tp = std::unique_ptr<int>; _Alloc = std::allocator<std::unique_ptr<int> >]'
main.cpp:6:8:   required from 'decltype(auto) entt::basic_registry< <template-parameter-1-1> >::pool_handler<Component>::replace(entt::basic_registry< <template-parameter-1-1> >&, Entity, Args&& ...) [with Args = {const noncopyable&}; Component = noncopyable; Entity = entt::entity]'
./entt/entt.hpp:8083:35:   required from 'decltype(auto) entt::basic_registry< <template-parameter-1-1> >::assign_or_replace(entt::basic_registry< <template-parameter-1-1> >::entity_type, Args&& ...) [with Component = noncopyable; Args = {const noncopyable&}; Entity = entt::entity; entt::basic_registry< <template-parameter-1-1> >::entity_type = entt::entity]'
./entt/entt.hpp:7445:21:   required from 'entt::basic_registry< <template-parameter-1-1> >::pool_type<Component>* entt::basic_registry< <template-parameter-1-1> >::assure() [with Component = noncopyable; Entity = entt::entity; entt::basic_registry< <template-parameter-1-1> >::pool_type<Component> = entt::basic_registry<entt::entity>::pool_handler<noncopyable>; typename std::decay<_Functor>::type = noncopyable]'
./entt/entt.hpp:7888:16:   required from 'decltype(auto) entt::basic_registry< <template-parameter-1-1> >::assign(entt::basic_registry< <template-parameter-1-1> >::entity_type, Args&& ...) [with Component = noncopyable; Args = {noncopyable}; Entity = entt::entity; entt::basic_registry< <template-parameter-1-1> >::entity_type = entt::entity]'
main.cpp:18:49:   required from here
C:/msys64/mingw64/include/c++/9.2.0/bits/stl_construct.h:75:7: error: use of deleted function 'std::unique_ptr<_Tp, _Dp>::unique_ptr(const std::unique_ptr<_Tp, _Dp>&) [with _Tp = int; _Dp = std::default_delete<int>]'
   75 |     { ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...); }
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from C:/msys64/mingw64/include/c++/9.2.0/memory:80,
                 from main.cpp:1:
C:/msys64/mingw64/include/c++/9.2.0/bits/unique_ptr.h:406:7: note: declared here
  406 |       unique_ptr(const unique_ptr&) = delete;
      | 

[skypjack]

The problem is that your type isn't movable. EnTT cannot create move constructors and assignment operators for you. Just define them and everything will work like a charm.

[markand]

Well, the standard says that copy/move constructors are automatically generated unless you create your own (the rules are more complex, but in this case it should work).

That's also why this works fine:

https://wandbox.org/permlink/tA9YrQN0H6VF2Cw8

Not sure why it didn't work with entt though...

[skypjack]

Good point. Let's reopen the issue. I'll look into it further. :+1:

[skypjack]

Ok, this is unexpected other than the source of the problem:

#include <type_traits>
#include <vector>
#include <memory>

struct noncopyable {
    std::vector<std::unique_ptr<int>> actions;
};

int main() {
    static_assert(std::is_copy_constructible_v<noncopyable>);
}

Shouldn't std::is_copy_constructible_v<noncopyable> be false? What's wrong in my reasoning?

[markand]

Yes, I'm not sure to understand why it reports true. I've tested it with clang, g++ and even MSVC all report true.

[skypjack]

Indeed, an this fails obviously:

int main() {
    static_assert(std::is_copy_constructible_v<noncopyable>);

    noncopyable foo;
    noncopyable bar{foo};
}

That is, it tells you that the type is copy constructible but then it returns an error you try to copy construct it. That's good. :smile:

This is pretty much the problem (without giving you the details of the internals). Any idea?

[markand]

It looks like it's the std::vector that wrongly report the copyable state of the POD. If using directly the unique_ptr as member variable, std::is_copy_constructible returns false;

#include <memory>
#include <vector>
#include <type_traits>

struct noncopyable {
        std::unique_ptr<int> x;
};

int main()
{
        printf("%d\n", std::is_copy_constructible_v<noncopyable>);
}

As expected:

$ ./a.exe
0

[markand]

Also, it can be simplified, for some reasons std::is_copy_constructible_v<std::vector<std::unique_ptr<int>>> also evaluates to true.

[skypjack]

Ok but it shouldn't be the case. An std::vector<std::unique_ptr<T>> isn't copyable because its object type isn't copyable as well. This is odd. I'm probably missing something obvious here...

[markand]

To my understanding, std::is_copy_constructible is probably misnomer. I think it only reports if the class has "rules" to be copied. For example, std::vector has a copy constructor defined explicitly (but you simply never use it with non-copyable objects) so that's probably why std::is_copy_constructible says true. While std::unique_ptr has explicitly deleted the copy constructor so std::is_copy_constructible has value of false.

In fact, I wonder if there is a way to really check if an underlying object can be copied no matter what copy constructors are defined, deleted or implicitly defined.

[skypjack]

At least we know you can make it work as:

struct noncopyable {
        noncopyable() = default;
        noncopyable(noncopyable &&) = default;
        noncopyable(const noncopyable &) = delete;
        noncopyable & operator=(const noncopyable &) = delete;
        noncopyable & operator=(noncopyable &&) = default;

        std::vector<std::unique_ptr<int>> actions;
};

However, it would be great to find a way to know that the type cannot be copied despite std::is_copy_constructible_v returns true.

[markand]

So we discussed this issue on both ##c++ and ##c++general and someone already asked this a long time ago (didn't find myself).

So I think the correct use case would be to use a custom SFINAE expression but I think this is a bit of ugly though...

Also interesting, someone proposed to change the behaviour of std::is_copy_constructible

The big downside of adding explicit copy/move constructors is the requirement to create a custom constructor just to initialize an aggregate type 😞

[skypjack]

I think I'll add an is_actually_copy_constructible trait to EnTT to work around this.
From what I can see it's a flaw in the standard and there is no plan to fix it because of the backward compatibility. So it's worth it to work around the issue directly in the library.
The traits seems pretty easy to implement after all.

[markand]

Can you show me the implementation? I've tried a custom SFINAE expression but could not get it to work. It's been a while since I've written one.

Thanks for looking up in this issue 😃

[skypjack]

Can you show me the implementation?

Sure. It will be available as part of the library once done. :wink:
If I forget to point it out here, ping me when you see this issue closed.

[skypjack]

Uhm, actually it's not that easy. I made a try.
It's easy to define a general purpose is_actually_copy_constructible but it won't intercept anyway something like in your case, where the non-copy constructible type is a data member of another type.
I don't see an easy way to do this but for that of deleting the copy constructor. Have you found a solution in the meantime?

[markand]

I have requested some comments on reddit.

[markand]

So they eventually said that the better option is still to simply disable the destructor explicitly. So I think we can close this issue. Is there a FAQ somewhere in the documentation we can add something about this?

The good thing is that disabling a destructor does not prevent aggregate initialization (the rules about that are fairly complex).

struct point {
    int x{0};
    int y{0};

    point(const point&) = delete;
};

point p{10, 10}; // still valid!

[skypjack]

There is a faq section in the wiki, it's updated with the faq.md file from the project.
Would you make a PR for it? Otherwise I can update the file myself, it's not a problem.

[markand]

Yes, I'll do it :-)