-
Notifications
You must be signed in to change notification settings - Fork 262
Restore Win7 compat by using old versions of threadpool functions #895
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
Conversation
Fortunately, the Ex versions behave the same as the Win7 versions, so we can just switch everybody to the Win7 versions.
kennykerr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet!
| void fire_immediately() noexcept | ||
| { | ||
| if (WINRT_IMPL_SetThreadpoolTimerEx(m_timer.get(), nullptr, 0, 0)) | ||
| if (WINRT_IMPL_SetThreadpoolTimer(m_timer.get(), nullptr, 0, 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to SetThreadpoolTimer docs, it returns void instead of BOOL, is it OK to use this return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted - @oldnewthing I'm guessing this means we don't have test coverage for this one. 😟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, because there's a cancellation test. Will have to figure out how this slipped through. Probably because I messed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, the deal is that this works on Win8+ due to how SetThreadpoolTimer is implemented (it calls SetThreadpoolTimerEx and throws away the result, but the result is still lying around in eax so we can still act on it). But this won't work on Win7, where eax does not contain anything meaningful. It looks like timer cancellation propagation will not work on Win7. Should I do GetProcAddress/dynamic light-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll have to do that. You can (almost) use load_runtime_function except that its currently only used for combase functions and the thread pool is implemented by kernel32.
cppwinrt/strings/base_agile_ref.h
Lines 124 to 140 in 1502e29
| template <typename F, typename L> | |
| void load_runtime_function(char const* name, F& result, L fallback) noexcept | |
| { | |
| if (result) | |
| { | |
| return; | |
| } | |
| result = reinterpret_cast<F>(WINRT_IMPL_GetProcAddress(WINRT_IMPL_LoadLibraryW(L"combase.dll"), name)); | |
| if (result) | |
| { | |
| return; | |
| } | |
| result = fallback; | |
| } |
This is the way Win7 fallback is handled, for example:
cppwinrt/strings/base_activation.h
Lines 21 to 38 in 1502e29
| inline int32_t __stdcall fallback_RoGetActivationFactory(void*, guid const&, void** factory) noexcept | |
| { | |
| *factory = nullptr; | |
| return error_class_not_available; | |
| } | |
| template <bool isSameInterfaceAsIActivationFactory> | |
| WINRT_IMPL_NOINLINE hresult get_runtime_activation_factory_impl(param::hstring const& name, winrt::guid const& guid, void** result) noexcept | |
| { | |
| if (winrt_activation_handler) | |
| { | |
| return winrt_activation_handler(*(void**)(&name), guid, result); | |
| } | |
| static int32_t(__stdcall * handler)(void* classId, winrt::guid const& iid, void** factory) noexcept; | |
| impl::load_runtime_function("RoGetActivationFactory", handler, fallback_RoGetActivationFactory); | |
| hresult hr = handler(*(void**)(&name), guid, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't quite do it that way because I want to disable a feature if the GetProcAddress fails. There is no "fallback" available. (I guess I could see if result == fallback_Blah.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you think is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fortunately, the Ex versions behave the same as the Win7 versions, so we can just switch everybody to the Win7 versions.
Fixes #894