[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits

[Toby Allsopp via Phabricator via cfe-commits]

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

[Toby Allsopp via Phabricator via cfe-commits]

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 &S, const FunctionProtoType *FnType,
+static QualType lookupPromiseType(Sema &S, 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

[PATCH] D35046: [coroutines] Include "this" type when looking up coroutine_traits

[Toby Allsopp via Phabricator via cfe-commits]

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