In order for libc++ to add <experimental/coroutine> to its module map, there has to be a feature that can be used to detect if coroutines support is enabled in Clang.
- rL304107: [coroutines] Support "coroutines" feature in module map requires clause
rC304054: [coroutines] Support "coroutines" feature in module map requires clause
rL304054: [coroutines] Support "coroutines" feature in module map requires clause
rC304107: [coroutines] Support "coroutines" feature in module map requires clause
rG0bb3bcd0ef96: [coroutines] Support "coroutines" feature in module map requires clause
rGe38cea026b92: [coroutines] Support "coroutines" feature in module map requires clause
Oh wait, I see what's going on. You're not testing for whether coroutines is enabled, you're testing for whether the __builtin_coro_* builtins exist. Are we sufficiently confident that those aren't going to change that we're prepared to make libc++ rely on this? (If we change the signature of those builtins in the future, then new versions of clang would stop being able to build old versions of the libc++ module.)
If we're not confident of that, how about calling the new feature something ugly like experimental_coroutines_builtins_20170525 or similar? That way, future versions of Clang can stop advertising that feature if we change the design of the builtins, and we can add a feature 'coroutines' later in a non-disruptive way if/when we decide we're happy with them as-is.
You can use a .cpp extension and the Modules TS import keyword if you prefer (add -fmodules-ts to the clang arguments), or use a .cpp extension and #pragma clang module import if you don't want to depend on a second TS in this test.
On reflection, I think this is fine. If the signatures of the builtins change, and the user builds with -fmodules -fcoroutines-ts and libc++, and there's version skew between libc++ and clang, they'll get a compile error. That's mostly the expected outcome; the issue is that we'd produce this compile error *even if* they never #include <experimental/coroutines>, because building the complete libc++ module would fail in that situation.
If we're worried about that, we could split the coroutines header out of the main libc++ module into its own top-level module, but I don't think we need to worry too much about rejecting code that uses -fcoroutines-ts but never actually uses a coroutine.
That's correct. I was mistaken as to why this was needed. I mistook a bug in libc++ for the reason this was needed.
So I have no need for this patch anymore.
Do you still want to land this for the reasons you mentioned?
r303936 will break the libc++ modules build if used with an older version of clang that doesn't have the coroutines builtins. If you're OK with that, then we don't need this. But if you intend to support older versions of Clang, then I think you need either this or a different approach (such as splitting out a separate top-level module for the coroutines header) to avoid that problem.
I'll have to investigate this further. I had assumed the builtins were added at the same time as the __cpp_coroutines macro, but if that's not the case libc++ could still guard the header correctly using either __has_builtin or the newly updated value of __cpp_coroutines; but a complete library solution seems possible.
For other users of Clang module maps, though, I see the convince of being able to do this within the module map.
Even after fixing the libc++ guards, the header still emits a #warning when it's processed when coroutines are unavailable.
It seems like a useful feature test to have available. I'll commit shortly.