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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In D108696#3082866 <https://reviews.llvm.org/D108696#3082866>, @ChuanqiXu wrote:

> @Quuxplusone gentle ping~

I think this PR is mostly above my pay grade. :)
IIUC, there is a chicken-and-egg problem between D108696 <https://reviews.llvm.org/D108696> and D109433 <https://reviews.llvm.org/D109433>? If I understand the situation correctly (which maybe I don't), I think you should add D108696 <https://reviews.llvm.org/D108696> as a "Parent Revision" of D109433 <https://reviews.llvm.org/D109433>, and tag both of them with //both// "libc++" //and// "clang" project tags, and then poke buildkite to re-run the CI on both of them. I would hope that D109433 <https://reviews.llvm.org/D109433>'s test results would still be green. And then you'd land both D108696 <https://reviews.llvm.org/D108696> and D109433 <https://reviews.llvm.org/D109433> in very quick succession. //But//, I'm not going to be an approver on D108696 <https://reviews.llvm.org/D108696> because it's over my head. Originally you pinged @rjmccall @lxfind @junparser ; are they still happy with D108696 <https://reviews.llvm.org/D108696> and the general direction we're taking on coroutines stuff in 14.x and 15.x?

(AIUI, the intent is for libc++ 14.x to support both C++11 `<experimental/coroutine>` and C++20 `<coroutine>`, and then in libc++ 15.x to support //only// C++20 `<coroutine>`.)



================
Comment at: clang/test/SemaCXX/coroutine_handle-addres-return-type.cpp:1
 // RUN: %clang_cc1 -verify %s -stdlib=libc++ -std=c++1z -fcoroutines-ts -fsyntax-only
 
----------------
Pre-existing: in the name of this file, `addres` should be `address`


================
Comment at: clang/test/SemaCXX/coroutines-exp-namespace.cpp:2
+// This file is the same with coroutines.cpp except the coroutine components are defined in std::experimental namespace.
+// This intention of this test is to make sure the legacy imeplementation in std::experimental namespace could work.
+// TODO: Remove this test once we didn't support
----------------
ldionne wrote:
> 
...and not just "could work," but "works." :)
```
// This file is the same as coroutines.cpp, except the components are defined in namespace std::experimental.
// The intent of this test is to make sure the std::experimental implementation still works.
// TODO: Remove this test once we drop support for <experimental/coroutine>.
```

Also, it occurs to me that you should probably be testing both `<coroutine>` and `<experimental/coroutine>` in all the other tests, as well; e.g. `coroutine_handle-address-return-type.cpp` should have a matching `coroutine_handle-address-return-type-exp-namespace.cpp` and so on. Otherwise, aren't you removing a lot of regression tests for `<experimental/coroutine>`, despite that we still claim to support `<experimental/coroutine>` in Clang 14.x?

In many cases, it should be possible to do something like
```
// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -fsyntax-only -verify %s -DINCLUDE_EXPERIMENTAL=1
// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s -DINCLUDE_EXPERIMENTAL=0

#if INCLUDE_EXPERIMENTAL
#include <experimental/coroutine>
namespace coro = std::experimental;
#else
#include <coroutine>
namespace coro = std;
#endif

[...]
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108696/new/

https://reviews.llvm.org/D108696