microsoft / cppwinrt

C++/WinRT
MIT License
1.64k stars 238 forks source link

It would be nice if event registrations could take `weak_from_this()` for shared_ptr in addition to `get_weak()` #1316

Closed dmachaj closed 1 year ago

dmachaj commented 1 year ago

Version

2.0.220131.2

Summary

class MyClass : public std::enable_shared_from_this<MyClass>
{
  MyClass() = default;
  void Initialize();
  void OnThingHappened(auto& thingSender, auto& thingArgs);
};

Somewhere in MyClass implementation:

void Initialize()
{
  const auto object = getObjectSomewhere();
  auto revoker = object.OnThing(winrt::auto_revoke, {weak_from_this(), &MyClass::OnThingHappened});
}

The code gen for the delegate is roughly:

    struct ThingHappenedHandler : winrt::Windows::Foundation::IUnknown
    {
        ThingHappenedHandler(std::nullptr_t = nullptr) noexcept {}
        ThingHappenedHandler(void* ptr, take_ownership_from_abi_t) noexcept : winrt::Windows::Foundation::IUnknown(ptr, take_ownership_from_abi) {}
        template <typename L> ThingHappenedHandler(L lambda);
        template <typename F> ThingHappenedHandler(F* function);
        template <typename O, typename M> ThingHappenedHandler(O* object, M method);
        template <typename O, typename M> ThingHappenedHandler(com_ptr<O>&& object, M method);
        template <typename O, typename M> ThingHappenedHandler(weak_ref<O>&& object, M method);
        auto operator()(winrt::ThingArgs const& args) const;
    };

It would be nice if that list of methods also had:

template <typename O, typename M> ThingHappenedHandler(std::weak_ptr<O>&& object, M method);

(This would need some sort of inclusion guard so that projects which do not include or use std::shared_ptr and std::weak_ptr will not require those headers to be included.)

The use case for this is a code base that is using pure C++ as much as possible while also calling some WinRT APIs. I want to be able to safely pass weak pointers for event callbacks to avoid races and use-after-free problems. However, I think that would require the class with the callback to derive from winrt::implements. That is possible but it would be ideal to not have to change it.

sylveon commented 1 year ago

This looks fairly easy to PR.

winrt/base.h already includes <memory> so everybody consuming cppwinrt is already forced into std::shared_ptr, the header guard isn't required.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

sylveon commented 1 year ago

I plan to PR this once we are able to again

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

sylveon commented 1 year ago

What's the opinion on this? Worth doing a PR?

dmachaj commented 1 year ago

I think it is, assuming it really is straightforward. I'm keeping it alive until I get a chance to take a deeper look (or someone else beats me to it). That'll probably be another 2-3 weeks, unfortunately.

github-actions[bot] commented 1 year ago

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

sylveon commented 1 year ago

Plan on looking at this in a week or so.