Skip to content

Conversation

@cpplearner
Copy link
Contributor

Fixes #2135.

Note: I noticed that cl defines __cpp_impl_coroutine when /await:strict is on, so I believe that there's no need to check defined(_DOWNLEVEL_COROUTINES_SUPPORTED). I also relocated the definition of __cpp_lib_coroutine to make it next to the other C++20 feature-test macro that's not guarded by _HAS_CXX20.

@cpplearner cpplearner requested a review from a team as a code owner February 1, 2022 12:16
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Feb 1, 2022

Note: I noticed that cl defines __cpp_impl_coroutine when /await:strict is on, so I believe that there's no need to check defined(_DOWNLEVEL_COROUTINES_SUPPORTED).

Confirmed. IIRC (maybe @joemmett remembers better) we waffled a bit on whether we wanted to define __cpp_impl_coroutine in sub-C++20 modes, but the final state of the compiler (from my reading) does seem to be that defined(_DOWNLEVEL_COROUTINES_SUPPORTED) is functionally determined by defined(__cpp_impl_coroutine) && _MSVC_LANG < 202002L.

When porting this change internally, we should add a commit to remove the _DOWNLEVEL_COROUTINES_SUPPORTED predefine from the compiler.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Feb 1, 2022
@joemmett
Copy link
Member

joemmett commented Feb 1, 2022

Note: I noticed that cl defines __cpp_impl_coroutine when /await:strict is on, so I believe that there's no need to check defined(_DOWNLEVEL_COROUTINES_SUPPORTED).

Confirmed. IIRC (maybe @joemmett remembers better) we waffled a bit on whether we wanted to defined __cpp_impl_coroutine in sub-C++20 modes, but the final state of the compiler (from my reading) does seem to be that defined(_DOWNLEVEL_COROUTINES_SUPPORTED) is functionally determined by defined(__cpp_impl_coroutine) && _MSVC_LANG < 202002L.

Correct. Looking back at the original PR the first implementation sensed _DOWNLEVEL_COROUTINES_SUPPORT directly in <coroutine> without either the impl or lib feature test macro being present. This was later changed to the current implementation.

When porting this change internally, we should add a commit to remove the _DOWNLEVEL_COROUTINES_SUPPORTED predefine from the compiler.

We can do that, we just need to make sure nobody is depending on this define (a quick search didn't turn up anything).

@StephanTLavavej StephanTLavavej self-assigned this Feb 2, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@CaseyCarter CaseyCarter merged commit 823dbe3 into microsoft:main Feb 7, 2022
@CaseyCarter
Copy link
Contributor

CaseyCarter commented Feb 7, 2022

Thanks for sweeping up this mess! 🧹

@cpplearner cpplearner deleted the feature-test-macros branch February 8, 2022 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

<yvals_core.h>: Cleanup conditional feature-test macro sections

4 participants