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