Page MenuHomePhabricator

[coroutines] Support "coroutines" feature in module map requires clause
ClosedPublic

Authored by EricWF on May 24 2017, 11:06 PM.

Diff Detail

Event Timeline

EricWF updated this revision to Diff 100207.May 24 2017, 11:06 PM
EricWF created this revision.
  • Alphabetize newly added switch case.
EricWF retitled this revision from [coroutines] Support "coroutines" feature to module map "requires" to [coroutines] Support "coroutines" feature in module map requires clause.May 24 2017, 11:11 PM
EricWF added inline comments.May 25 2017, 12:30 AM
test/Modules/requires-coroutines.mm
2

Should this test be called requires-coroutines.cpp or is using Obj-C++ a correct thing to do here?

rsmith edited edge metadata.May 25 2017, 3:55 PM

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

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.

test/Modules/requires-coroutines.mm
2

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.

rsmith accepted this revision.May 25 2017, 4:14 PM

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.)

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.

This revision is now accepted and ready to land.May 25 2017, 4:14 PM

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

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?

Also see r303936, which re-adds <experimental/coroutine> to the module map and fixes the bug.

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

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.

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

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.

EricWF closed this revision.May 26 2017, 7:46 PM
EricWF updated this revision to Diff 100567.May 28 2017, 2:01 PM

This patch was initially reverted due to a failing test it caused elsewhere; which has been address in this version.