[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
toby-allsopp abandoned this revision. toby-allsopp added a comment. Yes, @EricWF made a much more comprehensive change that has been in for a while now. Abandoning. https://reviews.llvm.org/D35046 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
GorNishanov added a comment. @toby-allsopp: You mentioned that @EricWF already got this in. Can you close ("abandon") this patch if it is no longer needed. https://reviews.llvm.org/D35046 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
toby-allsopp added a comment. In https://reviews.llvm.org/D35046#802838, @EricWF wrote: > 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? Will do, thanks for the suggestions. Comment at: lib/Sema/SemaCoroutine.cpp:85 +if (MD->isInstance()) { + QualType T = MD->getThisType(S.Context); + Args.addArgument(TemplateArgumentLoc( EricWF wrote: > 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 ]]. Oh wow, that's a howler. I will check my tests against MSVC here. Thanks. Comment at: lib/Sema/SemaCoroutine.cpp:441 + return false; + }(); EricWF wrote: > Huh, I've never seen lambdas used like this before but I really like it. It's a sometimes controversial style called immediately-invoked lambda/function expression (IILE or IIFE). I saw other instances of it in this file (or possibly some other file in clang) so I'm assuming it's acceptable style. https://reviews.llvm.org/D35046 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
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
[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits
toby-allsopp created this revision. In N4663, section [dcl.fct.def.coroutine] states that, in a coroutine that is a non-static member function, the type of the implicit object parameter is included immediately before the types of the function parameters. https://reviews.llvm.org/D35046 Files: lib/Sema/SemaCoroutine.cpp test/SemaCXX/coroutine-nonstatic-member-function.cpp Index: test/SemaCXX/coroutine-nonstatic-member-function.cpp === --- /dev/null +++ test/SemaCXX/coroutine-nonstatic-member-function.cpp @@ -0,0 +1,34 @@ +// RUN: %clang_cc1 -triple x86_64-apple-darwin9 %s -std=c++1z -fcoroutines-ts -fsyntax-only -Wall -Wextra -Wuninitialized -fblocks +#include "Inputs/std-coroutine.h" + +using namespace std::experimental; + +struct coro_t {}; + +struct coro_promise_type { + coro_t get_return_object() { return {}; } + suspend_never initial_suspend() { return {}; } + suspend_never final_suspend() { return {}; } + void return_void() {} + static void unhandled_exception() {} +}; + +struct C { + coro_t f(); +}; + +namespace std::experimental { + template<> + struct coroutine_traits{ +using promise_type = coro_promise_type; + }; +} + +coro_t C::f() { + co_return; +} + +int main() { + C c; + c.f(); +} Index: lib/Sema/SemaCoroutine.cpp === --- lib/Sema/SemaCoroutine.cpp +++ lib/Sema/SemaCoroutine.cpp @@ -13,6 +13,7 @@ #include "CoroutineStmtBuilder.h" #include "clang/AST/Decl.h" +#include "clang/AST/DeclCXX.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/StmtCXX.h" #include "clang/Lex/Preprocessor.h" @@ -43,9 +44,10 @@ /// Look up the std::coroutine_traits<...>::promise_type for the given /// function type. -static QualType lookupPromiseType(Sema , const FunctionProtoType *FnType, +static QualType lookupPromiseType(Sema , const FunctionDecl *FD, SourceLocation KwLoc, SourceLocation FuncLoc) { + const FunctionProtoType *FnType = FD->getType()->castAs(); // FIXME: Cache std::coroutine_traits once we've found it. NamespaceDecl *StdExp = S.lookupStdExperimentalNamespace(); if (!StdExp) { @@ -76,8 +78,15 @@ Args.addArgument(TemplateArgumentLoc( TemplateArgument(FnType->getReturnType()), S.Context.getTrivialTypeSourceInfo(FnType->getReturnType(), KwLoc))); - // FIXME: If the function is a non-static member function, add the type + // If the function is a non-static member function, add the type // of the implicit object parameter before the formal parameters. + if (auto *MD = dyn_cast_or_null(FD)) { +if (MD->isInstance()) { + QualType T = MD->getThisType(S.Context); + Args.addArgument(TemplateArgumentLoc( + TemplateArgument(T), S.Context.getTrivialTypeSourceInfo(T, KwLoc))); +} + } for (QualType T : FnType->getParamTypes()) Args.addArgument(TemplateArgumentLoc( TemplateArgument(T), S.Context.getTrivialTypeSourceInfo(T, KwLoc))); @@ -424,12 +433,17 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) { assert(isa(CurContext) && "not in a function scope"); auto *FD = cast(CurContext); + bool IsThisDependentType = [&] { +if (auto *MD = dyn_cast_or_null(FD)) + return MD->isInstance() && MD->getThisType(Context)->isDependentType(); +else + return false; + }(); QualType T = - FD->getType()->isDependentType() + FD->getType()->isDependentType() || IsThisDependentType ? Context.DependentTy - : lookupPromiseType(*this, FD->getType()->castAs(), - Loc, FD->getLocation()); + : lookupPromiseType(*this, FD, Loc, FD->getLocation()); if (T.isNull()) return nullptr; ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits