[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF updated this revision to Diff 100567.
EricWF added a comment.
This patch was initially reverted due to a failing test it caused elsewhere;
which has been address in this version.
https://reviews.llvm.org/D33538
Files:
docs/Modules.rst
lib/Basic/Module.cpp
test/Index/index-module.m
test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
test/Modules/Inputs/DependsOnModule.framework/module.map
test/Modules/requires-coroutines.mm
Index: test/Modules/requires-coroutines.mm
===
--- /dev/null
+++ test/Modules/requires-coroutines.mm
@@ -0,0 +1,12 @@
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules
-fimplicit-module-maps -F %S/Inputs %s -verify
+// RUN: %clang_cc1 -Wauto-import -fmodules-cache-path=%t -fmodules
-fimplicit-module-maps -F %S/Inputs %s -verify -fcoroutines-ts -DCOROUTINES
+
+
+#ifdef COROUTINES
+@import DependsOnModule.Coroutines;
+@import DependsOnModule.NotCoroutines; // expected-error {{module
'DependsOnModule.NotCoroutines' is incompatible with feature 'coroutines'}}
+#else
+@import DependsOnModule.NotCoroutines;
+@import DependsOnModule.Coroutines; // expected-error {{module
'DependsOnModule.Coroutines' requires feature 'coroutines'}}
+#endif
Index: test/Modules/Inputs/DependsOnModule.framework/module.map
===
--- test/Modules/Inputs/DependsOnModule.framework/module.map
+++ test/Modules/Inputs/DependsOnModule.framework/module.map
@@ -22,7 +22,14 @@
explicit module CustomReq2 {
requires custom_req2
}
-
+ explicit module Coroutines {
+requires coroutines
+header "coroutines.h"
+ }
+ explicit module NotCoroutines {
+requires !coroutines
+header "not_coroutines.h"
+ }
explicit framework module SubFramework {
umbrella header "SubFramework.h"
Index: test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
===
--- /dev/null
+++ test/Modules/Inputs/DependsOnModule.framework/Headers/not_coroutines.h
@@ -0,0 +1,3 @@
+#ifdef __cpp_coroutines
+#error coroutines must NOT be enabled
+#endif
Index: test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
===
--- /dev/null
+++ test/Modules/Inputs/DependsOnModule.framework/Headers/coroutines.h
@@ -0,0 +1,3 @@
+#ifndef __cpp_coroutines
+#error coroutines must be enabled
+#endif
Index: test/Index/index-module.m
===
--- test/Index/index-module.m
+++ test/Index/index-module.m
@@ -27,6 +27,7 @@
// CHECK-DMOD-NEXT: [ppIncludedFile]:
{{.*}}/Modules/Inputs/Module.framework{{[/\\]}}Headers{{[/\\]}}Module.h | name:
"Module/Module.h" | hash loc:
{{.*}}/Modules/Inputs/DependsOnModule.framework{{[/\\]}}Headers{{[/\\]}}DependsOnModule.h:1:1
| isImport: 0 | isAngled: 1 | isModule: 1 | module: Module
// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_OTHER_H:.*/Modules/Inputs/DependsOnModule\.framework[/\\]Headers[/\\]other\.h]]
| {{.*}} | hash loc: | {{.*}} | module: DependsOnModule
// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_NOT_CXX_H:.*/Modules/Inputs/DependsOnModule\.framework[/\\]Headers[/\\]not_cxx\.h]]
| {{.*}} | hash loc: | {{.*}} | module: DependsOnModule.NotCXX
+// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_NOT_CORO_H:.*/Modules/Inputs/DependsOnModule\.framework[/\\]Headers[/\\]not_coroutines\.h]]
| {{.*}} | hash loc: | {{.*}} | module: DependsOnModule.NotCoroutines
// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_SUB_H:.*/Modules/Inputs/DependsOnModule\.framework[/\\]Frameworks[/\\]SubFramework\.framework[/\\]Headers[/\\]SubFramework\.h]]
| {{.*}} | hash loc: | {{.*}} | module: DependsOnModule.SubFramework
// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_SUB_OTHER_H:.*/Modules/Inputs/DependsOnModule.framework[/\\]Frameworks[/\\]SubFramework\.framework[/\\]Headers[/\\]Other\.h]]
| name: "SubFramework/Other.h" | hash loc: [[DMOD_SUB_H]]:1:1 | isImport: 0 |
isAngled: 0 | isModule: 0 | module: DependsOnModule.SubFramework.Other
// CHECK-DMOD-NEXT: [ppIncludedFile]:
[[DMOD_PRIVATE_H:.*/Modules/Inputs/DependsOnModule.framework[/\\]PrivateHeaders[/\\]DependsOnModulePrivate.h]]
| {{.*}} | hash loc: | {{.*}} | module:
DependsOnModule.Private.DependsOnModule
Index: lib/Basic/Module.cpp
===
--- lib/Basic/Module.cpp
+++ lib/Basic/Module.cpp
@@ -64,6 +64,7 @@
bool HasFeature = llvm::StringSwitch(Feature)
.Case("altivec", LangOpts.AltiVec)
.Case("blocks", LangOpts.Blocks)
+.Case("coroutines", LangOpts.CoroutinesTS)
.Case("cplusplus", LangOpts.CPlusPlus)
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF added a comment. 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. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF added a comment. In https://reviews.llvm.org/D33538#765225, @rsmith wrote: > In https://reviews.llvm.org/D33538#765146, @EricWF wrote: > > > In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > > > > > Do we need to conditionalize this part of libc++? Nothing in the > > > 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. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
rsmith added a comment. In https://reviews.llvm.org/D33538#765146, @EricWF wrote: > In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > > > Do we need to conditionalize this part of libc++? Nothing in the > > 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. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF added a comment. Also see r303936, which re-adds to the module map and fixes the bug. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF added a comment. In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > Do we need to conditionalize this part of libc++? Nothing in the > 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? https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D33538#765062, @rsmith wrote: > In https://reviews.llvm.org/D33538#765045, @rsmith wrote: > > > Do we need to conditionalize this part of libc++? Nothing in the > > 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 `, 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. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
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 > 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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
rsmith added a comment. Do we need to conditionalize this part of libc++? Nothing in the header appears to need compiler support. https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D33538: [coroutines] Support "coroutines" feature in module map requires clause
EricWF added inline comments. 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 Should this test be called `requires-coroutines.cpp` or is using Obj-C++ a correct thing to do here? https://reviews.llvm.org/D33538 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
