microsoft / cppwinrt

C++/WinRT
MIT License
1.61k stars 232 forks source link

Simply static lifetime implementation #1375

Closed JaiganeshKumaran closed 5 months ago

JaiganeshKumaran commented 7 months ago

Implements #1300.

Please merge this pull request as I really need this feature.

JaiganeshKumaran commented 7 months ago

Another improvement can be done here - regarding constructors. I sometimes want to write the CreateInstance* functions manually due to have more than one implementation class for the runtime class.

JaiganeshKumaran commented 7 months ago

@kennykerr @oldnewthing

Link1J commented 7 months ago

I can't make sense of what optimizations this does. This breaks the current system to implement something that already is doable. Just put the method you want to implement on the factory_implementation as non-static. Which is what you do, so all this saves is the empty class with unimplemented methods? I can't see how this is helpful.

JaiganeshKumaran commented 7 months ago

@Link1J Static lifetime is a feature that ensures that the factory object is only created. This is useful for bundling additional state with the static class that needs to be initialised on demand.

Just put the method you want to implement on the factory_implementation as non-static.

Yes, that is already possible. However, when you use enable component optimisations[^1], it does not compile because the wrapper code in Class.g.cpp always accesses implementation::Class, implying that the functions must be present on it, which conflicts with static lifetime as you really want to put the static methods as non-static ones on the factory to access the state. Therefore, at the moment you need to manually write forwarder methods as noted here.

[^1]: Component optimisations provide 'inline' wrappers for static methods on the projected type instead of calling through the interface like usual.

kennykerr commented 7 months ago

Sorry, I've not had a moment to look at this.

jonwis commented 6 months ago

It feels a little hinky to rely on "call a static method on a null this kind of works..."

So you goal is to eliminate the "if you are using component optimizations [which everybody should be] then you will need to write forwarder methods" part?

JaiganeshKumaran commented 6 months ago

It feels a little hinky to rely on "call a static method on a null this kind of works..."

Yes, but it seems to be the simplest way to solve this problem. Also, note that there is no undefined behaviour because the method has to be static and C++ allows calling static methods of a type through a pointer or reference of type.

sylveon commented 6 months ago

Actually, it is. -> dereferences its left operand. Dereferencing a null pointer is UB.

JaiganeshKumaran commented 6 months ago

Actually, it is. -> dereferences its left operand. Dereferencing a null pointer is UB.

Okay, but it works so not going to care. It's not like C++/WinRT has no UB already. The alternate option would be to manually add the check for static lifetime in every single function.

Hmm actually it is not UB according to this answer: https://stackoverflow.com/a/63332797/12025335.

tim-weis commented 6 months ago

Okay, but it works so not going to care.

That's hard to prove (and by "hard" I mean "impossible"). I will admit that this is almost guaranteed to remain inconsequential with MSVC, given that it has to support MFC sporting its infamous CWnd::GetSafeHwnd() implementation. But Clang will most certainly be far less dovish in exploiting UB.

It's not like C++/WinRT has no UB already.

If that is the case, the reasonable response should be to remove the UB, not to add to it.

JaiganeshKumaran commented 6 months ago

Putting the following does not work, as only one branch is syntactically valid at a given time. Still think the nullptr trick is the best solution here.

 if constexpr (winrt::impl::has_static_lifetime_v<test_component::factory_implementation::StaticClass>)
 {
     winrt::make_self<test_component::factory_implementation::StaticClass>()->Method();
 }
 else
 {
     test_component::factory_implementation::StaticClass::statics_type::Method(); // could cause an error in case the first branch is the chosen one.
 }
sylveon commented 6 months ago

The entire point of if constexpr is allowing the branch not being picked to have invalid code

JaiganeshKumaran commented 6 months ago

The entire point of if constexpr is allowing the branch not being picked to have invalid code

All the branches should still be syntactically valid, unless the expressions are based on a template parameter (see these two examples: https://godbolt.org/z/MPc8KjxWd and https://godbolt.org/z/Ye3Txfqoz). In this case I cannot turn those functions into templates, however.

Wait I have an idea - use a lambda template! Hmm though it is a C++20 feature.

github-actions[bot] commented 6 months ago

This pull request 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.

JaiganeshKumaran commented 6 months ago

Even without static lifetime, you still may want to put static methods on the factory as static, so that you can have multiple implementations of the implementation type with different names.

github-actions[bot] commented 5 months ago

This pull request 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.