Skip to content

Conversation

@kobykahane
Copy link
Contributor

Fixes #764.

@kobykahane kobykahane marked this pull request as ready for review October 14, 2020 08:26
@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kennykerr
Copy link
Collaborator

Thanks Koby, this looks reasonable but I'd prefer to change the definition in one place rathe than duplicate it. Perhaps you can experiment with updating the following? This needs to support both C++20 and C++17 with the Visual C++ /await flag.

#if defined(_RESUMABLE_FUNCTIONS_SUPPORTED) && !defined(__cpp_coroutines)
#define __cpp_coroutines
#endif

@kobykahane
Copy link
Contributor Author

Thanks Koby, this looks reasonable but I'd prefer to change the definition in one place rathe than duplicate it. Perhaps you can experiment with updating the following? This needs to support both C++20 and C++17 with the Visual C++ /await flag.

#if defined(_RESUMABLE_FUNCTIONS_SUPPORTED) && !defined(__cpp_coroutines)
#define __cpp_coroutines
#endif

I assume you mean define a cppwinrt-specific macro here and use it, rather than define the Coroutines TS feature test macro in C++20 coroutines mode too, right? I'll update the PR accordingly.

@kennykerr
Copy link
Collaborator

Perhaps the base_macros.h would look something like this:

#if defined(_RESUMABLE_FUNCTIONS_SUPPORTED) && !defined(__cpp_lib_coroutine) 
#define __cpp_lib_coroutine 
#endif 

And then subsequent checks would simply test for the C++20 feature macro. I think that would work, although I haven't tested it.

@sylveon
Copy link
Contributor

sylveon commented Oct 15, 2020

Couldn't that mislead other libraries which use the macro for feature detection as well?

@kennykerr
Copy link
Collaborator

Perhaps - then we should define a WINRT_IMPL_COROUTINES macro.

You also need to account for this:

#ifdef __cpp_lib_coroutine
#include <coroutine>
namespace winrt::impl
{
template <typename T = void>
using coroutine_handle = std::coroutine_handle<T>;
using suspend_always = std::suspend_always;
using suspend_never = std::suspend_never;
}
#else
#include <experimental/coroutine>
namespace winrt::impl
{
template <typename T = void>
using coroutine_handle = std::experimental::coroutine_handle<T>;
using suspend_always = std::experimental::suspend_always;
using suspend_never = std::experimental::suspend_never;
}
#endif

@kennykerr
Copy link
Collaborator

Of course, if we can fix base_macros.h to not define a feature macro and then update the rest, as Koby originally did, to rely only on those feature macros that would save introducing yet another macro.

@kobykahane kobykahane force-pushed the fix_cpp20_coro branch 2 times, most recently from bfe7f8b to c2f577d Compare October 15, 2020 17:43
@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kobykahane
Copy link
Contributor Author

@kennykerr Any additional comments?

@kennykerr
Copy link
Collaborator

That looks great - I've spun a build. If all looks good I'll merge this in - thanks!

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - thanks Koby!

@kennykerr kennykerr merged commit ed1079a into microsoft:master Oct 15, 2020
@kobykahane kobykahane deleted the fix_cpp20_coro branch October 15, 2020 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Windows.Foundation await adapters do not compile with C++20 standard coroutines

3 participants