[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

[Chuanqi Xu via Phabricator via cfe-commits]

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