EricWF requested changes to this revision. EricWF added a comment. This revision now requires changes to proceed.
I think the test could be improved. First could you add the test within `test/SemaCXX/coroutines.cpp`? Second could you add some negative tests that check the diagnostics generated when you don't provide a specialization of coroutine traits (ie that the old behaviour of not including the class type now produces diagnostics). Could you also add tests for const/volatile and lvalue/rvalue qualified member functions? ================ Comment at: lib/Sema/SemaCoroutine.cpp:85 + if (MD->isInstance()) { + QualType T = MD->getThisType(S.Context); + Args.addArgument(TemplateArgumentLoc( ---------------- This seems wrong to me. `getThisType` returns the type of the `this` parameter as specified under [class.this] but according to the coroutines spec the type of the parameter should be the type of the `implicit object parameter`, which is specified under [[http://eel.is/c++draft/over.match.funcs#4 | (over.match.funcs) p4 ]]. ================ Comment at: lib/Sema/SemaCoroutine.cpp:441 + return false; + }(); ---------------- Huh, I've never seen lambdas used like this before but I really like it. https://reviews.llvm.org/D35046 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits