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

ChuanqiXu added a comment. @lxfind @Quuxplusone @lewissbaker Thanks for looking into this! I have committed this. Since the remained problem is that whether or not should we support the case that user uses "std::coroutine*" and "std::experimental::coroutine*" at the same time. As the reason we

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

lxfind accepted this revision. lxfind added a comment. I also agree that we should try to keep the compiler simple and not support the complicated case. It should be fairly straightforward for a codebase to update fully to use std instead of std::experimental (we have a large coroutine codebase

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

ChuanqiXu added a comment. In D108696#3096822 , @Quuxplusone wrote: > In D108696#3095789 , @ChuanqiXu > wrote: > >> Since there are other `experimental/*` headers moved in to normal include >> paths, I guess

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

Quuxplusone added a comment. In D108696#3095789 , @ChuanqiXu wrote: > 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.

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

ChuanqiXu marked 5 inline comments as done. ChuanqiXu added a comment. In D108696#3090484 , @Quuxplusone wrote: > @lewissbaker wrote: > >> #include // which transitively includes >> #include > > Good example! I had not previously been thinking

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

Quuxplusone added a comment. @lewissbaker wrote: > #include // which transitively includes > #include 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 `` and `` in the same

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

ChuanqiXu added a comment. In D108696#3086011 , @lewissbaker wrote: > So am I correct in understanding that the main issue with the chicken/egg > problem for updating both the compiler to use the new stdlib facilities and > updating the stdlib

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

lewissbaker added a comment. So am I correct in understanding that the main issue with the chicken/egg problem for updating both the compiler to use the new stdlib facilities and updating the stdlib facilities is that we don't want to issue warnings about using `` and telling users to use ``

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

ChuanqiXu added inline comments. 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

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

ChuanqiXu added a comment. In D108696#3083081 , @Quuxplusone wrote: > In D108696#3082866 , @ChuanqiXu > wrote: > >> @Quuxplusone gentle ping~ > > I think this PR is mostly above my pay grade. :) Sorry for

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

Quuxplusone added a comment. In 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 and D109433

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

ChuanqiXu added a comment. @Quuxplusone gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ChuanqiXu added a comment. In D108696#3069723 , @avogelsgesang wrote: > Hi @ChuanqiXu > > sorry for what might be naive questions, but just to make sure I understand > the context of this patch correctly: Hi, you are welcome : ) > This patch fixes

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

avogelsgesang added a comment. Hi @ChuanqiXu sorry for what might be naive questions, but just to make sure I understand the context of this patch correctly: This patch fixes the look up of the `coroutine_traits`, so that clang now search both the `std` and the `std::experimental` namespace.

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

ChuanqiXu added a comment. @ldionne @Quuxplusone gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

ChuanqiXu added a comment. In D108696#3018045 , @ldionne wrote: > This sounds more reasonable to me, however we need to ship `` in > libc++ before we enable this, or else we're going to start suggesting that > users include `` when we don't have it.

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

ldionne added a comment. This sounds more reasonable to me, however we need to ship `` in libc++ before we enable this, or else we're going to start suggesting that users include `` when we don't have it. Comment at: clang/lib/Sema/SemaCoroutine.cpp:1668 +if

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

ChuanqiXu added a comment. @ldionne @Quuxplusone Now the compiler wouldn't emit warning for any more. So I think this is good. Do you feel good with this? This is necessary to conform the implementation of coroutine since the compiler would search coroutine components in corresponding

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

ChuanqiXu planned changes to this revision. ChuanqiXu added a comment. We should proceed after D108697 accepted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696

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

ChuanqiXu added a comment. In D108696#2983021 , @ldionne wrote: > Based on the commit description, I don't understand this change at all. Why > do we want to tweak the name lookup just for `std::coroutine`? Yes, we do > have an action item to finish

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

ldionne reopened this revision. ldionne added a comment. This revision is now accepted and ready to land. (woops, this closed the rev when I committed the revert) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/

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

ldionne reopened this revision. ldionne added a comment. This revision is now accepted and ready to land. Based on the commit description, I don't understand this change at all. Why do we want to tweak the name lookup just for `std::coroutine`? Yes, we do have an action item to finish

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

gulfem added a comment. We started seeing the following test failures in our Fuchsia builds after this patch: Failed Tests (10): libc++ :: libcxx/experimental/language.support/support.coroutines/dialect_support.pass.cpp libc++ ::

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

uabelho added a comment. This patch makes several libcxx tests fail due to the new warning. See e.g. http://lab.llvm.org:8011/#/builders/60/builds/4632 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696

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

lxfind accepted this revision. lxfind added a comment. This revision is now accepted and ready to land. LGTM. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list

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

ChuanqiXu added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11015 + "coroutine_traits defined in std::experimental namespace is deprecated. " + "Considering update libcxx and include instead of .">, + InGroup; avogelsgesang

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

avogelsgesang added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11015 + "coroutine_traits defined in std::experimental namespace is deprecated. " + "Considering update libcxx and include instead of .">, + InGroup;

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

ChuanqiXu added a comment. @rjmccall @lxfind @junparser hi, do you feel comfortable with this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org