Entt: Issue in entt::registry::clear() member function

[nitishingde]

#include "entt/entt.hpp"
#include <iostream>

struct CompA {
    int *index;
    ~CompA() {
        if(index) {
            delete index;
            index = nullptr;
            std::cout<<"DELETED| CompA\n";
        }
    }
};

struct CompB {
    int *index;

    ~CompB() {
        if(index) {
            delete index;
            index = nullptr;
            std::cout << "DELETED| CompB\n";
        }
    }
};

int main() {
    auto reg = entt::registry();
    auto entity = reg.create();
    auto &a = reg.emplace<CompA>(entity);
    a.index = new int(42);
    auto &b = reg.emplace<CompB>(entity);
    b.index = new int(40);

    reg.clear();
//    reg.clear<CompA>();
    return 0;
}

Problem:
reg.clear<CompA>() calls the destructor on every component CompA (it's working as intended).
However, reg.clear() calls the destructor twice on each and every component. (issue)

In the discord @skypjack mentioned

It's not a problem for itself, one of the destructors is that of a temporary object that is moved in a swap

Sorry, but I disagree. Let's take CompA as an example. For either 1 of the 2 objects (main or temporary), the index member variable becomes a dangling pointer (whichever is not destroyed the 1st time).

[codylico]

_(outsider comments; feel free to ignore)_ How are the copy/move assignment/constructors* of struct CompA and struct CompB handling the int* field? If they aren't handling the int* field, then any copy/move, performed by entt or otherwise, may break the struct.

See discussions on the rule of three in C++.

*copy assignment operator (CompA::operator=(const CompA&)), copy constructor (CompA::CompA(const CompA&)), move assignment operator (CompA::operator=(CompA&&)), move constructor (CompA::CompA(CompA&&))

[skypjack]

Yup, that's not a problem. EnTT requires all types to be movable. The fact that a pointer data member isn't properly invalidated when moving the object and thus incurs a double free is definitely not an EnTT problem.
Though, I've asked to open the issue to keep track of the fact that a call to clear() can be improved. It currently iterates all entities and destroy them one at a time. Instead, we can use range functionalities to clear all pools. This should be way faster, especially when there are no listeners (the vast majority of cases), since it's O(1) or so. 👍

[skypjack]

Available upstream on experimental. I'll merge it on master soon.