[PATCH] D48863: [Sema] Explain coroutine_traits template in diag

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9053
   "a coroutine">;
+def note_coroutine_types_for_traits_here : Note<
+  "the coroutine traits class template is being instantiated using the return "
----------------
modocache wrote:
> GorNishanov wrote:
> > I am wondering what is the use case here.
> > Is it to guard against badly designed standard library?
> > 
> > I played around a little and tried to see how I could get to trigger this error with a user-code, but, failed. If I did not specify enough parameters it will default to primary template defined in the <experimental/coroutine>
> That's fair. Here's my rationale: SemaCoroutune.cpp contains diagnostics, even before this change, in case of a "badly designed standard library". There's a diagnostic for if coroutine_traits is defined as non-template class, for example.
> 
> A user would not encounter that diagnostic under any normal circumstance. If they follow the instructions the compiler gives them, they'll include `<experimental/coroutine>` and be done with it. But since we have the code to emit the (unlikely to ever be seen) diagnostic anyway, why not make it as helpful as we can?
> 
> If a user for some reason defines their own `coroutine_traits`, and their definition only takes a single template argument, and if they define a coroutine with a single return type and a single parameter, then they'll see the diagnostic "too many template arguments for class template". At this point to learn why the heck a `co_return` statement is instantiating a template, they'd have to either read the compiler source code or the standard. My rationale is that we might as well save them this step.
> 
> All that being said, this change is just a suggestion, I'm not insisting we put it in! I drew some inspiration for this change from your Naked Coroutines talk, in which you used the `co_return` keyword and then followed the instructions the compiler gave you until you had a working program. Since the Clang diagnostic tells me in that case that "std::experimental::coroutine_traits is not defined, you must include <experimental/coroutine>", I wondered what would happen if a user decided to just define their own `std::experimental::coroutine_traits` instead. If they do that, and correct the errors that `coroutine_traits` must be a class and it must be a template, eventually they could find their way to this diagnostic. I agree that it's an unlikely case, so feel free to reject this and I'll close it :)
Rather than try to explain after the fact what happened, I think we should do some up-front checking that the `coroutine_traits` template we found looks reasonable. Specifically: walk its template parameter list, and check that it has a non-pack type parameter followed by a pack type parameter. If not, diagnose that we don't support that declaration for `coroutine_traits` with some kind of "unsupported standard library implementation" error.

But the purpose of this would be to check that we're being used with a standard library implementation that we understand, not to allow arbitrary end users to declare `coroutine_traits` themselves. (Maybe one day we should add a diagnostic for users writing code in namespace `std`...)


Repository:
  rC Clang

https://reviews.llvm.org/D48863