ChuanqiXu added a comment. In D108696#3083081 <https://reviews.llvm.org/D108696#3083081>, @Quuxplusone wrote:
> In D108696#3082866 <https://reviews.llvm.org/D108696#3082866>, @ChuanqiXu > wrote: > >> @Quuxplusone gentle ping~ > > I think this PR is mostly above my pay grade. :) Sorry for disturbing. I would remind it. > 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>`.) Yeah, this is a Parent Revision of D109433 <https://reviews.llvm.org/D109433>. But I didn't tag this with 'libc++' since it didn't touch libcxx part. Do you mean it is necessary to tag it as 'libc++' even if it didn't touch libcxx? (I would try to make D109433 <https://reviews.llvm.org/D109433> green, of course) This revision is approved originally but reverted due to the unwanted warning. I would ask them if they are happy with the current direction. After all, thanks for looking into this : ) ================ 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 ---------------- Quuxplusone wrote: > 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 > > [...] > ``` Thanks for reporting this. I would try to test both by duplicating the tests. The method to use macro may not be good due to it needs to mangled. 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
