Quuxplusone added a comment. @lewissbaker wrote:
> #include <other/header.hpp> // which transitively includes <coroutine> > #include <experimental/coroutine> Good example! I had not previously been thinking about transitive includes. I believe we "obviously" don't need to cater to code that manually includes both `<coroutine>` and `<experimental/coroutine>` in the same source file; but transitive includes are //vastly// more likely to happen in practice, and so if we're not going to care about //them//, that's a policy decision. Might still be a good tradeoff, to break some code in the short term in exchange for a simpler compiler (also in the short term), but its goodness is not //obvious.// > The only way I can think of making this work is to just make > `std::experimental::*` an alias for `std::*`. > But that only works for `std::experimental::coroutine_handle`. It doesn't > work for `std::experimental::coroutine_traits` as you can't add > specialisations through an alias. We //could// use a `using`-declaration to bring `std::coroutine_traits` into `namespace std::experimental`. That works, and you can still add specializations for `std::experimental::coroutine_traits<U>` because that's just a //name// that looks-up-to the same template. https://godbolt.org/z/fWGrT5js5 However, as shown in that godbolt, this would have the (salutary) effect of breaking users who are out there (in the year of our lord 2021!) still reopening `namespace std` just to add a template specialization. But! My understanding is that the only reason we're keeping `<experimental/coroutine>` alive at all, in 14.x, is to provide continuity for its users and not break their existing code right away. If we go changing the API of `<experimental/coroutine>` (by aliasing it to `<coroutine>`), then we //do// break those users right away (because their code depends on the old experimental API, not the new conforming one). So "alias it to `<coroutine>`" doesn't seem like a viable path forward, anyway. (Also, `<coroutine>` wants to use C++20-only features, but `<experimental/coroutine>` must continue to work in C++14.) I think we need to start from the premise that `<experimental/coroutine>` and `<coroutine>` will have different APIs; and if that makes it difficult to support Lewis's very reasonable transitive-include scenario, then we have to either implement something difficult, or else make a policy decision that 14.x simply won't support translation units that transitively include both APIs. (15.x certainly will not support such TUs, because it won't support //any// TUs that include `<experimental/coroutine>`, transitively or otherwise.) IOW, it sounds like we're all (@ChuanqiXu @lewissbaker @Quuxplusone) reluctantly OK with the resolution "Do not support translation units that transitively include both APIs"; but it would be helpful to have someone more authoritative weigh in (with either "yes that's OK policy" or "no we //need// to find some other solution"), if such a person is watching. ================ Comment at: clang/docs/LanguageExtensions.rst:2872 https://wg21.link/P0057. The following four are intended to be used by the -standard library to implement `std::experimental::coroutine_handle` type. +standard library to implement `std::coroutine_handle` type. ---------------- ``` to implement the ``std::coroutine_handle`` type. ``` ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11002 def err_coroutine_handle_missing_member : Error< - "std::experimental::coroutine_handle missing a member named '%0'">; + "std::coroutine_handle missing a member named '%0'">; def err_malformed_std_coroutine_traits : Error< ---------------- Pre-existing: It's weird that the surrounding messages are of the form "foo must be bar," and then this one is "foo isn't baz". This message //could// be re-worded as `std::coroutine_handle must have a member named '%0'` for consistency. (But that might be out of scope for this PR.) ================ Comment at: clang/include/clang/Sema/Sema.h:10274 + /// Lookup 'coroutine_traits' in std namespace and std::experimental + /// namespace. The namespace found would be recorded in Namespace. ClassTemplateDecl *lookupCoroutineTraits(SourceLocation KwLoc, ---------------- `s/would be/is/` ================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:1668 + if (!CoroNamespace || !LookupQualifiedName(Result, CoroNamespace)) { + /// TODO: Lookup in std::expeirmental namespace for compability. + /// Remove this once users get familiar with coroutine under std ---------------- ldionne wrote: > ``` /// Look up in namespace std::experimental, for compatibility. /// TODO: Remove this extra lookup when <experimental/coroutine> is removed. ``` (The extra lookup is done, not TODO. The //removal// is the TODO part. Also, grammar nits.) ================ Comment at: clang/test/AST/coroutine-source-location-crash.cpp:12-14 #include "Inputs/std-coroutine.h" +using namespace std; ---------------- Thanks for adding `-exp-namespace` versions of the Sema tests. I think you should do the same for all of these tests as well, for the same reason: we don't want to remove test coverage related to `std::experimental` until those codepaths are actually gone. Also, just as a practical matter: This test says it runs in `c++14` mode, but our actual `std::coroutine_handle` relies on C++20isms. So, my conclusion is that right now you've got a file `Inputs/std-coroutine.h` that is //claiming// to be a mockup of libc++'s `std::coroutine_handle`, but //actually// is a mockup of `std::experimental::coroutine_handle`. That's going to be really confusing. I think we need both `Inputs/std-experimental-coroutine.h` and `Inputs/std-coroutine.h`, and we need `-exp-namespace` copies of all these tests too. As a bonus, this will also allow you to test the compiler's behavior on a TU like ``` #include "Inputs/std-coroutine.h" #include "Inputs/std-experimental-coroutine.h" struct my_awaitable { bool await_ready(); // expected-error@1 {{'await_suspend' must be callable with 'std::coroutine_handle<>' or whatever}} void await_suspend(std::experimental::coroutine_handle<> coro); void await_resume(); }; ``` in order to prove (and regression-test) that the compiler does not "support" this scenario but also does not crash! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
