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

[Chuanqi Xu via Phabricator via cfe-commits]

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 stated above, it is hard to implement and the help is limited. 
Even in the case that someone points out necessary and strong reasons that we 
need to support both "std::coroutine*" and "std:::experimental::coroutine". We 
could support it in later patches.


Repository:
  rG LLVM Github Monorepo

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

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

[Xun Li via Phabricator via cfe-commits]

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 as well). 
Given that everyone is mostly supportive, I will accept the change.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 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 ?
>
> I'm not aware of any similar situations involving ``. Most 
> stuff in `` is just library stuff, like 
> `std::experimental::pmr`, where the two versions can happily coexist because 
> we have namespaces. Coroutines are special (and really annoying) because 
> they're a core-language feature with a "magic" relationship to a specific 
> library header. (Core-language syntax doesn't respect namespaces.) The only 
> other "magic" features like that are
>
> - `typeid` looks for `std::type_info`
> - `{}` looks for `std::initializer_list`
> - `auto [a,b]` looks for `std::tuple_size` etc.
> - `<=>` looks for `std::strong_ordering` etc.
>
> None of these were ever prototyped in `` before being put 
> into the Standard, so they never ran into this problem.

Oh, got it. So now is the time to do policy decision and the community didn't 
meet similar problem before. Here is my reason for not supporting both:

- It is complex to implement.
- It would be removed in recent future. Complexity is not a very strong reason 
to refuse a need. But it is frustrating to implement a feature which would be 
removed in a predictable recent future.
- From the perspective of users, it shouldn't be hard to switch from 
std::experimental::coro* to std::coro*. I write coroutine codes in production 
too. AFAIK, it should be easy. (I understand that I couldn't stand for all the 
users)

Here are my reasons. But if we couldn't get in consensus. I guess we could only 
post this to cfe-dev to ask more people for involving in.


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

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

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

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. So how does libc++ do before for this situation 
> @Quuxplusone ?

I'm not aware of any similar situations involving ``. Most 
stuff in `` is just library stuff, like 
`std::experimental::pmr`, where the two versions can happily coexist because we 
have namespaces. Coroutines are special (and really annoying) because they're a 
core-language feature with a "magic" relationship to a specific library header. 
(Core-language syntax doesn't respect namespaces.) The only other "magic" 
features like that are

- `typeid` looks for `std::type_info`
- `{}` looks for `std::initializer_list`
- `auto [a,b]` looks for `std::tuple_size` etc.
- `<=>` looks for `std::strong_ordering` etc.

None of these were ever prototyped in `` before being put into 
the Standard, so they never ran into this problem.


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

[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 , @Quuxplusone 
wrote:

> @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 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` 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 
> `` 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 `` (by aliasing it to ``), 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 ``" 
> doesn't seem like a viable path forward, anyway. (Also, `` wants 
> to use C++20-only features, but `` must continue to 
> work in C++14.)  I think we need to start from the premise that 
> `` and `` 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 ``, 
> 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

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

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

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 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` 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 
`` 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 `` (by aliasing it to ``), 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 ``" 
doesn't seem like a viable path forward, anyway. (Also, `` wants to 
use C++20-only features, but `` must continue to work 
in C++14.)  I think we need to start from the premise that 
`` and `` 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 ``, 
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.




Comment at: clang/docs/LanguageExtensions.rst:2872
 https://wg21.link/P0057. The following four are intended to be used by the
-standard library to implement `std::experimental::coroutine_handle` type.
+standard library to implement `std::coroutine_handle` type.
 

```
to implement the ``std::coroutine_handle`` type.
```



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<

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.)



Comment at: clang/include/clang/Sema/Sema.h:10274
+  /// Lookup 'coroutine_traits' in std namespace and std::experimental
+  /// namespace. The namespace found would be recorded in Namespace.
   ClassTemplateDecl *lookupCoroutineTraits(SourceLocation KwLoc,

`s/would be/is/`



Comment at: clang/lib/Sema/SemaCoroutine.cpp:1668
+if (!CoroNamespace || !LookupQualifiedName(Result, CoroNamespace)) {
+  /// TODO: Lookup in std::expeirmental namespace for compability.
+  /// Remove this once users get familiar with coroutine under std

ldionne wrote:
> 
```
/// Look up in namespace std::experimental, for compatibility.
/// TODO: Remove this extra lookup when  is removed.
```
(The extra lookup is done, not TODO. The //removal// is the TODO part. Also, 
grammar nits.)



Comment at: clang/test/AST/coroutine-source-location-crash.cpp:12-14
 #include "Inputs/std-coroutine.h"
 
+using namespace std;

Thanks for adding `-exp-namespace` versions of the Sema tests. I think you 
should do the same for all of these tests as well, for the same reason: we 
don't want to remove test coverage related to 

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 facilities is that we don't want to issue warnings about 
> using `` and telling users to use `` 
> instead if there is no `` header.
>
> I wonder if a suitable transition approach here might be to have the compiler 
> do:
>
> - try lookup of std::coroutine_handle/coroutine_traits; if found try to 
> instantiate the expression (e.g. await-suspend) using the std:: type
> - if not found then try lookup of 
> std::experimental::coroutine_handle/coroutine_traits; and if found try to 
> instantiate the expression using the std::experimental:: type; and if 
> successful, then;
>   - if std:: type was found but instantiating the expression failed, then 
> issue a warning about using deprecated std::experimental:: types
>   - otherwise if std:: type was not found, try to find  header and 
> if present then issue a warning suggesting using  instead of 
> 
>
> Then you should be able to land the compiler change and it should continue to 
> have the same behaviour for existing code using std::experimental:: types 
> with the existing stdlib.
> It is only once the stdlib changes that land which add the  header 
> that users would start seeing the warnings about using  instead of 
> 

It looks odd for me with the sentence:

> otherwise if std:: type was not found, try to find  header and if 
> present then issue a warning suggesting using  instead of 
> 

If we could find the  header, we should be able to find `std::coro*` 
types. Otherwise, the  header is fake.

My transition plan would be:

- Land this.
- Land D109433 , it is the change who added 
.
- Land another patch in clang to issue a warning for the deprecated use of 
. The reason why we need to split this with the first 
step is that we don't want to trigger the CI system for libc++. (It's the 
reason this patch is rejected before).
- Remove  in libcxx in LLVM15.
- Remove the support for `std::experimental::coro*` in clang in LLVM16.



> Note that there are still some potential breakages for code that could go on 
> here, however.
>
> Imagine that some header I included has been updated to use  while 
> my current header is still using  so that both are in 
> scope.
>
> I might have defined an awaitable type that looks like the following:
>
>   c++
>   #include  // which transitively includes 
>   #include 
>   
>   struct my_awaitable {
> bool await_ready();
> void await_suspend(std::experimental::coroutine_handle<> coro);
> void await_resume();
>   };
>
> In this case, the compiler would find the `std::coroutine_handle<>` and try 
> to instantiate the await-suspend expression with that type, which would fail 
> because `await_suspend()` is not callable with a `std::coroutine_handle`.
> The compiler would need to fall back to instantiating the expression with 
> `std::experimental::coroutine_handle`.

My personal opinion is that this case should fail instead of requiring the 
compiler to fall back to instantiating with `std::experimental::coro*`. There 
are two reasons:

- It is not easy to implement such fall back to instantiating logics. It would 
make the current search process much more complex.
- It is not worth to do so in my opinion. Since  is the 
deprecated use. It is odd for me that we need to do extra work to conform it.

> There is also the variation where await_suspend() is a template function that 
> deduces the promise-type argument.
> e.g.
>
>   c++
>   template
>   void await_suspend(std::experimental::coroutine_handle coro);
>
> There are also cases where the template parameter to await_suspend() is 
> unconstrained.
> e.g.
>
>   c++
>   struct promise_type {
> ...
> std::experimental::coroutine_handle<> continuation;
>   };
>   
>   struct my_awaitable {
> bool await_ready();
> template
> auto await_suspend(CoroHandle handle) {
>   coro.promise().continuation = handle;
>   return coro;
> }
> void await_resume();
>   
> std::experimental::coroutine_handle coro;
>   };
>
> Such a type would successfully deduce the template parameter using 
> std::coroutine_handle and so the await-suspend expression would be valid, but 
> the instantiation of the await_suspend() body would fail as a 
> std::coroutine_handle cannot be assigned to an lvalue of type 
> `std::experimentl::coroutine_handle<>`. This would not produce a 
> SFINAE-friendly error that the compiler could retry with std::experimental 
> types.
>
> I'm not sure if there's a great way of keeping such code working in a mixed 
> `` and `` world.
> Maybe the cost of doing so is not worth the benefit?

Thanks for offering these examples. It is valuable. But I think the root cause 
may be the mixed use for `` and ``. It is 

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

[Lewis Baker via Phabricator via cfe-commits]

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 `` instead 
if there is no `` header.

I wonder if a suitable transition approach here might be to have the compiler 
do:

- try lookup of std::coroutine_handle/coroutine_traits; if found try to 
instantiate the expression (e.g. await-suspend) using the std:: type
- if not found then try lookup of 
std::experimental::coroutine_handle/coroutine_traits; and if found try to 
instantiate the expression using the std::experimental:: type; and if 
successful, then;
  - if std:: type was found but instantiating the expression failed, then issue 
a warning about using deprecated std::experimental:: types
  - otherwise if std:: type was not found, try to find  header and 
if present then issue a warning suggesting using  instead of 


Then you should be able to land the compiler change and it should continue to 
have the same behaviour for existing code using std::experimental:: types with 
the existing stdlib.
It is only once the stdlib changes that land which add the  header 
that users would start seeing the warnings about using  instead of 


Note that there are still some potential breakages for code that could go on 
here, however.

Imagine that some header I included has been updated to use  while 
my current header is still using  so that both are in 
scope.

I might have defined an awaitable type that looks like the following:

  c++
  #include  // which transitively includes 
  #include 
  
  struct my_awaitable {
bool await_ready();
void await_suspend(std::experimental::coroutine_handle<> coro);
void await_resume();
  };

In this case, the compiler would find the `std::coroutine_handle<>` and try to 
instantiate the await-suspend expression with that type, which would fail 
because `await_suspend()` is not callable with a `std::coroutine_handle`.
The compiler would need to fall back to instantiating the expression with 
`std::experimental::coroutine_handle`.

There is also the variation where await_suspend() is a template function that 
deduces the promise-type argument.
e.g.

  c++
  template
  void await_suspend(std::experimental::coroutine_handle coro);

There are also cases where the template parameter to await_suspend() is 
unconstrained.
e.g.

  c++
  struct promise_type {
...
std::experimental::coroutine_handle<> continuation;
  };
  
  struct my_awaitable {
bool await_ready();
template
auto await_suspend(CoroHandle handle) {
  coro.promise().continuation = handle;
  return coro;
}
void await_resume();
  
std::experimental::coroutine_handle coro;
  };

Such a type would successfully deduce the template parameter using 
std::coroutine_handle and so the await-suspend expression would be valid, but 
the instantiation of the await_suspend() body would fail as a 
std::coroutine_handle cannot be assigned to an lvalue of type 
`std::experimentl::coroutine_handle<>`. This would not produce a 
SFINAE-friendly error that the compiler could retry with std::experimental 
types.

I'm not sure if there's a great way of keeping such code working in a mixed 
`` and `` world.
Maybe the cost of doing so is not worth the benefit?

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.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 imeplementation in 
std::experimental namespace could work.
+// TODO: Remove this test once we didn't support

ChuanqiXu wrote:
> 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 .
> > ```
> > 
> > Also, it occurs to me that you should probably be testing both 
> > `` and `` 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 
> > ``, despite that we still claim to support 
> > `` 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 
> > namespace coro = std::experimental;
> > #else
> > #include 
> > 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.
(Ignore this) I just noticed that we could use the macro based method for Sema 
tests.


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 disturbing. I would remind it.

> IIUC, there is a chicken-and-egg problem between D108696 
>  and D109433 
> ? If I understand the situation correctly 
> (which maybe I don't), I think you should add D108696 
>  as a "Parent Revision" of 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 
> 's test results would still be green. And 
> then you'd land both D108696  and D109433 
>  in very quick succession. //But//, I'm not 
> going to be an approver on D108696  because 
> it's over my head. Originally you pinged @rjmccall @lxfind @junparser ; are 
> they still happy with 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 
> `` and C++20 ``, and then in libc++ 15.x 
> to support //only// C++20 ``.)

Yeah, this is a Parent Revision of 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  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 .
> ```
> 
> Also, it occurs to me that you should probably be testing both `` 
> and `` 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 
> ``, despite that we still claim to support 
> `` 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 
> namespace coro = std::experimental;
> #else
> #include 
> 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

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

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

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 
? If I understand the situation correctly 
(which maybe I don't), I think you should add D108696 
 as a "Parent Revision" of 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 's test 
results would still be green. And then you'd land both D108696 
 and D109433 
 in very quick succession. //But//, I'm not 
going to be an approver on D108696  because 
it's over my head. Originally you pinged @rjmccall @lxfind @junparser ; are 
they still happy with 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 
`` and C++20 ``, and then in libc++ 15.x to 
support //only// C++20 ``.)




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 .
```

Also, it occurs to me that you should probably be testing both `` 
and `` 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 ``, 
despite that we still claim to support `` 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 
namespace coro = std::experimental;
#else
#include 
namespace coro = std;
#endif

[...]
```


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 the look up of the `coroutine_traits`, so that clang now 
> search both the `std` and the `std::experimental` namespace. As such, it must 
> land before D109433  since otherwise the 
> new `std::coroutine_traits` wouldn't be found by the compiler, correct?

Yes, correct.

> Also, does this patch enable me to use `clang-cl` to build programs which use 
> the coroutine header from Microsoft's STL 
> ? Or are there 
> still other incompatibilities between clang and the Microsoft STL with 
> regards to coroutines?

After this patch, it should be able to compile  MS STL. Otherwise, 
there must be a bug in either the clang part or in the MS STL part.


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

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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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. As such, it must 
land before D109433  since otherwise the new 
`std::coroutine_traits` wouldn't be found by the compiler, correct?

Also, does this patch enable me to use `clang-cl` to build programs which use 
the coroutine header from Microsoft's STL 
? Or are there 
still other incompatibilities between clang and the Microsoft STL with regards 
to coroutines?


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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.

We couldn't ship '' before we enable this. Otherwise the compiler 
couldn't find coroutine components defined in `std` namespace.
The patch before tries to emit warning for user who including 
 but it broke the CI system of libcxx. That's the 
reason that the first commit get reverted.
I think the right order may be:

- Check in this.
- Then check in D109433 .
- Add a warning to tell people that they should include  instead of 
. This patch should be simple enough to check in 
quickly.

How do you think about this order?


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

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

[Louis Dionne via Phabricator via cfe-commits]

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 (!CoroNamespace || !LookupQualifiedName(Result, CoroNamespace)) {
+  /// TODO: Lookup in std::expeirmental namespace for compability.
+  /// Remove this once users get familiar with coroutine under std





Comment at: clang/lib/Sema/SemaCoroutine.cpp:1677
   }
+  /// TODO: Add warning here once we updates libcxx.
+}

Add a warning about what?



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




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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 namespace 
(std::experimental before and both std and std::experimental 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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 coroutines in libc++ already, and I'd love to 
> see a patch that does that, but I don't think that mandates changing Clang.
>
> The rollout plan for coroutines should be:
>
> 1. Make sure we implement coroutines fully
> 2. Duplicate it all into namespace `std`
> 3. In two LLVM releases, remove all the coroutines stuff in 
> `std::experimental`.
>
> I'm going to revert this for now on the basis that it breaks libc++ CI. Let's 
> have a discussion about the above if you think I'm mistaken or if I'm 
> misunderstanding what this patch does.

Many thanks for reverting this!

> Why do we want to tweak the name lookup just for `std::coroutine`? Yes, we do 
> have an action item to finish coroutines in libc++ already

This is due to a practice that:

- People want to use clang but they prefer to use libstdc++ instead of libc++.
- So that they would implement a `coroutine.h` by coping from libc++. So that 
they could depend on libstdc++ still.

This is the reason that I introduce the warning in the compiler side instead of 
libc++. But if this warning matters, I am Ok to emit the warning in the 
compiler side.

Then let's talk about the plans to move coroutine components into std namespace:

In D108697 , the successor of this patch, we 
had duplicated all the components into namespace `std`. (And I add an warning 
in `std::experimental` by #warning directive, I am not sure if this warning may 
break the CI)
Then here is the problem, we must commit this before D108697 
, otherwise the compiler wouldn't search 
coroutine components in `std`  namespace.
The solution I got now is that I should remove the warning message in this 
patch. Then the compiler would still try to lookup in  `std::experimental` 
namespace without warning if it can't find things in `std` namespace.
Do you @ldionne  @Quuxplusone think this solution workable?


Repository:
  rG LLVM Github Monorepo

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

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

[Louis Dionne via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D108696

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Louis Dionne via Phabricator via cfe-commits]

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 coroutines in libc++ already, and I'd love to see a patch 
that does that, but I don't think that mandates changing Clang.

The rollout plan for coroutines should be:

1. Make sure we implement coroutines fully
2. Duplicate it all into namespace `std`
3. In two LLVM releases, remove all the coroutines stuff in `std::experimental`.

I'm going to revert this for now on the basis that it breaks libc++ CI. Let's 
have a discussion about the above if you think I'm mistaken or if I'm 
misunderstanding what this patch does.


Repository:
  rG LLVM Github Monorepo

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

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

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

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++ :: 
std/experimental/language.support/support.coroutines/coroutine.handle/coroutine.handle.prom/promise.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/await_result.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/bool_await_suspend.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/expected.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/fullexpr-dtor.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/generator.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/go.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/multishot_func.pass.cpp
libc++ :: 
std/experimental/language.support/support.coroutines/end.to.end/oneshot_func.pass.cpp

https://logs.chromium.org/logs/fuchsia/buildbucket/cr-buildbucket/8837144494402864417/+/u/clang/test/stdout

Do you have any idea what might be going wrong?


Repository:
  rG LLVM Github Monorepo

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

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

[Mikael Holmén via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xun Li via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Chuanqi Xu via Phabricator via cfe-commits]

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 wrote:
> Considering update -> Consider updating
Done. Thanks for suggestion!


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

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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

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;

Considering update -> Consider updating


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

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

[Chuanqi Xu via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits