-
Notifications
You must be signed in to change notification settings - Fork 262
Some uses of co_await were not protected by WINRT_IMPL_COROUTINES
#1002
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
strings/base_coroutine_foundation.h
Outdated
| private: | ||
| static fire_and_forget cancel_asynchronously(Async async) | ||
| { | ||
| #ifdef WINRT_IMPL_COROUTINES |
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.
Why not call submit_threadpool_callback instead of changing the semantics of this function?
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, I had a note to make this fix better, but this is to unblock "cannot build because of compiler errors" which is significantly more urgent.
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.
Could this type just be excluded entirely? Seems only to be used by operator co_await further down in the file (which is also conditioned on WINRT_IMPL_COROUTINES).
As is, I'd expect this to still give compilation errors, but of the form "function has no return"
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, I went and did it.
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.
Let's try to avoid further feature creep. It's useful to have fixes stand alone in PRs so they can easily be rolled back and reapplied if needed.
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.
The previous fix was incomplete. It kept the compiler happy but didn't preserve behavior.
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.
@dunhor Ooh, good point. I missed that it's used only by co_await. Let me see what happens if I take it out entirely.
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.
Got it 👍
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.
@dunhor Okay, it works. I had to comment out a boatload of tests that used co_await, but the rest of them build successfully when WINRT_IMPL_COROUTINES is disabled.
Fixes #1001