ChuanqiXu marked 5 inline comments as done. ChuanqiXu added a comment. In D108696#3090484 <https://reviews.llvm.org/D108696#3090484>, @Quuxplusone wrote:
> @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. Yeah, the last key point here may be the problem that how do we treat for programs which contains both APIs. Since there are other `experimental/*` headers moved in to normal include paths, I guess there may be similar problems before. I think this problem is not limited in coroutine. So how does libc++ do before for this situation @Quuxplusone ? Since it looks like that people here may agree that we don't support both APIs. So I add an error if the compiler founds the mixed use of `std::coro*` and `std::experimental::coro*`. I think this is the best we could do if we decide to not support it. ================ 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< ---------------- Quuxplusone wrote: > 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.) Oh, many thanks for the detailed reviews. I edit this to the way you introduced. Otherwise we couldn't remember this defect after this patch. 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
