MyGUI / mygui

Fast, flexible and simple GUI.
http://mygui.info/
Other
713 stars 205 forks source link

Use unique_ptr in Delegate #256

Closed Assumeru closed 1 year ago

Assumeru commented 1 year ago

Replaces manual memory management and its corresponding destructors with std::unique_ptr. MultiDelegate::mListDelegates can no longer contain nullptrs, removing the need for mutable.

I noticed that MultiDelegate::clear(IDelegateUnlink*) and MultiDelegate::operator-=(IDelegate*) are effectively the same function with the latter taking ownership of its argument pointer. Ideally it'd take an std::unique_ptr<IDelegate>&& argument instead (and the same goes for Delegate's assignment operator and MultiDelegate's +=) but that'd probably involve making newDelegate return std::unique_ptr as well, which would be a larger change.

Altren commented 1 year ago

Nice. I guess the next step would be changing MyGUI::newDelegate, so that it return unique_ptr and new/delete is no longer called in the delegates related code

Altren commented 1 year ago

By the way, there is one downside in this change: now we instantiate std::unique_ptr for each delegate type and in typical project there are plenty of them. I'll do some investigation in a couple of days to make sure that this is not a significant issue.

Assumeru commented 1 year ago

Ideally a compiler would produce the exact same output as before, but that's probably too optimistic. A minimal godbolt version shows a reduction in the number of lines of assembly output by GCC at -O1, but that also doesn't mean much. https://godbolt.org/z/acTPGKh1r https://godbolt.org/z/vxP6Wz197

Depending on how common mListDelegates manipulation is, it might be worthwhile to switch from std::list to std::vector.

Altren commented 1 year ago

The scenario I'm talking about is when you have multiple different delegates. Try to create 5 different delegates (with different signature) in your test.

Assumeru commented 1 year ago

This still compares favourably in terms of output size 🤷

template<class... T>
using Del = MyGUI::delegates::MultiDelegate<T...>;

int main(int argc, const char** argv)
{
    Del<const char*> md;
    Del<char, int> md2;
    Del<int, double, float> md3;
    Del<const void*, short, short, long> md4;
    Del<double, unsigned, signed, long, short> md5;
    md += MyGUI::newDelegate([] (const char* str) {
        std::cout << str << '\n';
    }, 1);
    md2 += MyGUI::newDelegate([&] (char a, int b) {
        md4(nullptr, a, a, b);
    }, 2);
    md3 += MyGUI::newDelegate([&] (int a, double b, float c) {
        for(int i = 0; i < a; ++i) {
            md5(b, a, a, long(c), 2);
        }
    }, 3);
    md4 += MyGUI::newDelegate([&] (const void* a, short b, short c, long d) {
        if (a) {
            md += MyGUI::newDelegate([=] (const char* str) {
                std::cout << str << c << d << '\n';
            }, 10 * b * c * d);
        }
    }, 4);
    md5 += MyGUI::newDelegate([&] (double a, unsigned b, signed c, long d, short e) {
        std::cout << a << b << c << d << e << '\n';
    }, 5);
    for(int i = 0; i < argc; ++i) {
        md(argv[i]);
        for(char c : std::string_view{argv[i]})
            md2(c, i);
        md3(i, 2. * i, 0.f);
        md4(static_cast<const void*>(argv[i]), 0, 1, i);
        md5(1.5 * i, i, -i, i, 1);
    }
    md3.clear();
    md -= MyGUI::newDelegate([] (const char* str) {
        std::cout << str << '\n';
    }, 1);
    for(int i = 0; i < argc; ++i)
        md(argv[i]);
}
Altren commented 1 year ago

That's great. This mean that unique_ptr is either more simple that the deleted lines or compiler eliminates it well enough. Anyway this proves that I was wrong about downside of your change, which is good

Altren commented 1 year ago

It seems, that there is regression in this change. LayoutEditor crashes at start. Reverting this changes fixes the issue