[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause

[Richard Smith via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D33538#765045, @rsmith wrote:

> Do we need to conditionalize this part of libc++? Nothing in the <coroutines> header appears to need compiler support.


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.



================
Comment at: test/Modules/requires-coroutines.mm:1
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules -fimplicit-module-maps -F %S/Inputs %s -verify
----------------
EricWF wrote:
> Should this test be called `requires-coroutines.cpp` or is using Obj-C++ a correct thing to do here?
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.


https://reviews.llvm.org/D33538