Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simply static lifetime implementation #1375

Closed

Conversation

JaiganeshKumaran
Copy link
Contributor

Implements #1300.

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

@JaiganeshKumaran
Copy link
Contributor Author

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
Copy link
Contributor Author

@kennykerr @oldnewthing

@Link1J
Copy link

Link1J commented Dec 15, 2023

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
Copy link
Contributor Author

@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 optimisations1, 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.

Footnotes

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

@kennykerr
Copy link
Collaborator

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

@jonwis
Copy link
Member

jonwis commented Dec 22, 2023

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?

cppwinrt/component_writers.h Outdated Show resolved Hide resolved
@JaiganeshKumaran
Copy link
Contributor Author

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
Copy link
Contributor

sylveon commented Dec 23, 2023

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

@JaiganeshKumaran
Copy link
Contributor Author

JaiganeshKumaran commented Dec 23, 2023

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

sylveon commented Dec 27, 2023

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

@JaiganeshKumaran
Copy link
Contributor Author

JaiganeshKumaran commented Dec 28, 2023

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.

Copy link

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
Copy link
Contributor Author

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.

@JaiganeshKumaran JaiganeshKumaran marked this pull request as draft January 14, 2024 06:20
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants