ETLCPP / etl

Embedded Template Library
https://www.etlcpp.com
MIT License
2.17k stars 386 forks source link

Allow `etl::delegate::create()` to accept instance pointer for member functions? #810

Closed jaskij closed 4 months ago

jaskij commented 9 months ago

This is probably the one counter intuitive part of etl::delegate that always catches me, whenever I use it. The member function variant of create() only accepts references, not pointers. More so that I quite often pass this as the argument.

Would making an overload accepting pointers, with perhaps an assert to check for null pointers, make sense?

jwellbelove commented 9 months ago

I looked into this request some time ago, but unfortunately, due to the way that etl::delgate works, it cannot accept runtime function pointers.

It can be done if a proxy class is created with a member function that calls the stored run time function pointer. The compile time address of this function can be sent to the etl::delgate.

I do have an implementation of a close clone of std::function (which will replace the deprecated current etl::function), but I have not integrated into the main branch as of yet.

jaskij commented 9 months ago

I looked into this request some time ago, but unfortunately, due to the way that etl::delgate works, it cannot accept runtime function pointers.

Ah, sorry, I didn't explain it clearly enough. No, the function would be compile-time. What would be run time is the class instance.

Right now, etl::delegate::create() supports taking the class instance by reference, from the guide:

etl::delegate<int(int)> d = etl::delegate<int(int)>::create<Test, &Test::member_function>(test);

What I am asking is just an overload, which allows the test argument to be Test*, as compared to the existing Test&.

jwellbelove commented 9 months ago

OK, I understand.

Why not just use *this?

jaskij commented 9 months ago

Oh, I do, but I don't use etl::delegate often enough for this to be intuitive, so I always loose twenty-thirty minutes figuring out build errors whenever this comes up. So this is simply a quality-of-life thing.

Alternatively, maybe just update the documentation here to clearly show that test is being passed by reference?

image

jwellbelove commented 9 months ago

It would be fairly easy to make the delegate to also accept pointers to class instances. I'll put it on my TODO list.

jaskij commented 9 months ago

I'll put it on my TODO list.

I'll make a PR, hopefully before Christmas, sounds like a great first issue to get my feet wet with contributing.

jwellbelove commented 9 months ago

Don't forget there are <= C++03 and >= C++11 versions. And remember to add the relevant unit tests.

jaskij commented 8 months ago

Between Christmas chaos and my mental state, I utterly forgot about this, sorry. I'll get to it over the weekend.

jwellbelove commented 8 months ago

No worries, I'm having trouble getting various ETL work done at the moment, as I'm busy renovating the kitchen and other rooms in the house.