[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
GorNishanov marked an inline comment as done.
Closed by commit rL294933: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt 
construction. (authored by GorNishanov).

Changed prior to commit:
  https://reviews.llvm.org/D28835?vs=88152=88156#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28835

Files:
  cfe/trunk/include/clang/AST/StmtCXX.h
  cfe/trunk/include/clang/Sema/ScopeInfo.h
  cfe/trunk/lib/AST/StmtCXX.cpp
  cfe/trunk/lib/Sema/SemaCoroutine.cpp
  cfe/trunk/test/SemaCXX/coroutines.cpp

Index: cfe/trunk/include/clang/Sema/ScopeInfo.h
===
--- cfe/trunk/include/clang/Sema/ScopeInfo.h
+++ cfe/trunk/include/clang/Sema/ScopeInfo.h
@@ -157,7 +157,7 @@
   SmallVector Returns;
 
   /// \brief The promise object for this coroutine, if any.
-  VarDecl *CoroutinePromise;
+  VarDecl *CoroutinePromise = nullptr;
 
   /// \brief The list of coroutine control flow constructs (co_await, co_yield,
   /// co_return) that occur within the function or block. Empty if and only if
Index: cfe/trunk/include/clang/AST/StmtCXX.h
===
--- cfe/trunk/include/clang/AST/StmtCXX.h
+++ cfe/trunk/include/clang/AST/StmtCXX.h
@@ -296,7 +296,9 @@
 /// \brief Represents the body of a coroutine. This wraps the normal function
 /// body and holds the additional semantic context required to set up and tear
 /// down the coroutine frame.
-class CoroutineBodyStmt : public Stmt {
+class CoroutineBodyStmt final
+: public Stmt,
+  private llvm::TrailingObjects {
   enum SubStmt {
 Body,  ///< The body of the coroutine.
 Promise,   ///< The promise statement.
@@ -309,52 +311,76 @@
 ReturnValue,   ///< Return value for thunk function.
 FirstParamMove ///< First offset for move construction of parameter copies.
   };
-  Stmt *SubStmts[SubStmt::FirstParamMove];
+  unsigned NumParams;
 
   friend class ASTStmtReader;
+  friend TrailingObjects;
+
+  Stmt **getStoredStmts() { return getTrailingObjects(); }
+
+  Stmt *const *getStoredStmts() const { return getTrailingObjects(); }
+
 public:
-  CoroutineBodyStmt(Stmt *Body, Stmt *Promise, Stmt *InitSuspend,
-Stmt *FinalSuspend, Stmt *OnException, Stmt *OnFallthrough,
-Expr *Allocate, Stmt *Deallocate,
-Expr *ReturnValue, ArrayRef ParamMoves)
-  : Stmt(CoroutineBodyStmtClass) {
-SubStmts[CoroutineBodyStmt::Body] = Body;
-SubStmts[CoroutineBodyStmt::Promise] = Promise;
-SubStmts[CoroutineBodyStmt::InitSuspend] = InitSuspend;
-SubStmts[CoroutineBodyStmt::FinalSuspend] = FinalSuspend;
-SubStmts[CoroutineBodyStmt::OnException] = OnException;
-SubStmts[CoroutineBodyStmt::OnFallthrough] = OnFallthrough;
-SubStmts[CoroutineBodyStmt::Allocate] = Allocate;
-SubStmts[CoroutineBodyStmt::Deallocate] = Deallocate;
-SubStmts[CoroutineBodyStmt::ReturnValue] = ReturnValue;
-// FIXME: Tail-allocate space for parameter move expressions and store them.
-assert(ParamMoves.empty() && "not implemented yet");
-  }
+
+  struct CtorArgs {
+Stmt *Body = nullptr;
+Stmt *Promise = nullptr;
+Expr *InitialSuspend = nullptr;
+Expr *FinalSuspend = nullptr;
+Stmt *OnException = nullptr;
+Stmt *OnFallthrough = nullptr;
+Expr *Allocate = nullptr;
+Expr *Deallocate = nullptr;
+Stmt *ReturnValue = nullptr;
+ArrayRef ParamMoves;
+  };
+
+private:
+
+  CoroutineBodyStmt(CtorArgs const& Args);
+
+public:
+  static CoroutineBodyStmt *Create(const ASTContext , CtorArgs const );
 
   /// \brief Retrieve the body of the coroutine as written. This will be either
   /// a CompoundStmt or a TryStmt.
   Stmt *getBody() const {
-return SubStmts[SubStmt::Body];
+return getStoredStmts()[SubStmt::Body];
   }
 
-  Stmt *getPromiseDeclStmt() const { return SubStmts[SubStmt::Promise]; }
+  Stmt *getPromiseDeclStmt() const {
+return getStoredStmts()[SubStmt::Promise];
+  }
   VarDecl *getPromiseDecl() const {
 return cast(cast(getPromiseDeclStmt())->getSingleDecl());
   }
 
-  Stmt *getInitSuspendStmt() const { return SubStmts[SubStmt::InitSuspend]; }
-  Stmt *getFinalSuspendStmt() const { return SubStmts[SubStmt::FinalSuspend]; }
+  Stmt *getInitSuspendStmt() const {
+return getStoredStmts()[SubStmt::InitSuspend];
+  }
+  Stmt *getFinalSuspendStmt() const {
+return getStoredStmts()[SubStmt::FinalSuspend];
+  }
 
-  Stmt *getExceptionHandler() const { return SubStmts[SubStmt::OnException]; }
+  Stmt *getExceptionHandler() const {
+return getStoredStmts()[SubStmt::OnException];
+  }
   Stmt *getFallthroughHandler() const {
-return SubStmts[SubStmt::OnFallthrough];
+return getStoredStmts()[SubStmt::OnFallthrough];
   }
 
-  Expr *getAllocate() const { return cast(SubStmts[SubStmt::Allocate]); }
-  

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov marked an inline comment as done.
GorNishanov added a comment.

Preparing to land. Thank you very much for review!




Comment at: lib/Sema/SemaCoroutine.cpp:719
+
+  if (!PromiseRecordDecl)
+return true;

EricWF wrote:
> I figured out what's going on. `PromiseRecordDecl` doesn't get initialized to 
> null when `IsPromiseDependentType` is false. Initializing `PromiseRecordDecl` 
> fixes the problem.
Good catch!


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 88152.
GorNishanov added a comment.

Initialized PromiseRecordDecl to nullptr


https://reviews.llvm.org/D28835

Files:
  include/clang/AST/StmtCXX.h
  include/clang/Sema/ScopeInfo.h
  lib/AST/StmtCXX.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include }}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -487,7 +487,7 @@
 static bool buildAllocationAndDeallocation(Sema , SourceLocation Loc,
FunctionScopeInfo *Fn,
Expr *,
-   Stmt *) {
+   Expr *) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
@@ -564,6 +564,48 @@
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema 
+  FunctionDecl 
+  FunctionScopeInfo 
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl = nullptr;
+
+public:
+  SubStmtBuilder(Sema , FunctionDecl , FunctionScopeInfo , Stmt *Body)
+  : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+IsPromiseDependentType(
+!Fn.CoroutinePromise ||
+Fn.CoroutinePromise->getType()->isDependentType()) {
+this->Body = Body;
+if (!IsPromiseDependentType) {
+  PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+  assert(PromiseRecordDecl && "Type should have already been checked");
+}
+this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+makeFinalSuspend() && makeOnException() &&
+makeOnFallthrough() && makeNewAndDeleteExpr() &&
+makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
@@ -577,120 +619,159 @@
 << (isa(First) ? 0 :
 isa(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-  ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+  S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->Promise = PromiseStmt.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-  buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+  buildPromiseCall(S, , Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+  InitialSuspend = S.ActOnFinishFullExpr(InitialSuspend.get());
   if (InitialSuspend.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->InitialSuspend = InitialSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeFinalSuspend() {
   // Form and check implicit 'co_await p.final_suspend();' statement.
   ExprResult FinalSuspend =
-  buildPromiseCall(*this, Fn, Loc, "final_suspend", None);
+  buildPromiseCall(S, , Loc, "final_suspend", None);
   // FIXME: Support operator co_await here.
   if (!FinalSuspend.isInvalid())
-FinalSuspend = BuildCoawaitExpr(Loc, FinalSuspend.get());
-  FinalSuspend = 

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 88151.
GorNishanov added a comment.

Initialized member variable to zero.


https://reviews.llvm.org/D28835

Files:
  include/clang/AST/StmtCXX.h
  include/clang/Sema/ScopeInfo.h
  lib/AST/StmtCXX.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include }}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -487,7 +487,7 @@
 static bool buildAllocationAndDeallocation(Sema , SourceLocation Loc,
FunctionScopeInfo *Fn,
Expr *,
-   Stmt *) {
+   Expr *) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
@@ -564,6 +564,48 @@
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema 
+  FunctionDecl 
+  FunctionScopeInfo 
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl;
+
+public:
+  SubStmtBuilder(Sema , FunctionDecl , FunctionScopeInfo , Stmt *Body)
+  : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+IsPromiseDependentType(
+!Fn.CoroutinePromise ||
+Fn.CoroutinePromise->getType()->isDependentType()) {
+this->Body = Body;
+if (!IsPromiseDependentType) {
+  PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+  assert(PromiseRecordDecl && "Type should have already been checked");
+}
+this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+makeFinalSuspend() && makeOnException() &&
+makeOnFallthrough() && makeNewAndDeleteExpr() &&
+makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }  
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
@@ -577,120 +619,159 @@
 << (isa(First) ? 0 :
 isa(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-  ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+  S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->Promise = PromiseStmt.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-  buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+  buildPromiseCall(S, , Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+  InitialSuspend = S.ActOnFinishFullExpr(InitialSuspend.get());
   if (InitialSuspend.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->InitialSuspend = InitialSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeFinalSuspend() {
   // Form and check implicit 'co_await p.final_suspend();' statement.
   ExprResult FinalSuspend =
-  buildPromiseCall(*this, Fn, Loc, "final_suspend", None);
+  buildPromiseCall(S, , Loc, "final_suspend", None);
   // FIXME: Support operator co_await here.
   if (!FinalSuspend.isInvalid())
-FinalSuspend = BuildCoawaitExpr(Loc, FinalSuspend.get());
-  FinalSuspend = 

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

This LGTM after applying the fixes.




Comment at: lib/Sema/SemaCoroutine.cpp:719
+
+  if (!PromiseRecordDecl)
+return true;

I figured out what's going on. `PromiseRecordDecl` doesn't get initialized to 
null when `IsPromiseDependentType` is false. Initializing `PromiseRecordDecl` 
fixes the problem.


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.

This currently segfaults on my machine. Here is the full output of running 
`SemaCXX/coroutines.cpp`.
https://gist.github.com/EricWF/81dc332e21c3e5c6bdc024cda87b846f

I'm not sure exactly what's going on, but it should be fixed before committing.


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov added a comment.

Gentle and melodic ping.


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov added a comment.

LGTM? Pretty please :)


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov marked 2 inline comments as done.
GorNishanov added a comment.

Looks even better now!


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 85490.
GorNishanov added a comment.

Feedback implemented!


https://reviews.llvm.org/D28835

Files:
  include/clang/AST/StmtCXX.h
  include/clang/Sema/ScopeInfo.h
  lib/AST/StmtCXX.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include }}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -487,7 +487,7 @@
 static bool buildAllocationAndDeallocation(Sema , SourceLocation Loc,
FunctionScopeInfo *Fn,
Expr *,
-   Stmt *) {
+   Expr *) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
@@ -564,6 +564,48 @@
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema 
+  FunctionDecl 
+  FunctionScopeInfo 
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl;
+
+public:
+  SubStmtBuilder(Sema , FunctionDecl , FunctionScopeInfo , Stmt *Body)
+  : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+IsPromiseDependentType(
+!Fn.CoroutinePromise ||
+Fn.CoroutinePromise->getType()->isDependentType()) {
+this->Body = Body;
+if (!IsPromiseDependentType) {
+  PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+  assert(PromiseRecordDecl && "Type should have already been checked");
+}
+this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+makeFinalSuspend() && makeOnException() &&
+makeOnFallthrough() && makeNewAndDeleteExpr() &&
+makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
@@ -577,120 +619,159 @@
 << (isa(First) ? 0 :
 isa(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-  ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+  S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->Promise = PromiseStmt.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-  buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+  buildPromiseCall(S, , Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+  InitialSuspend = S.ActOnFinishFullExpr(InitialSuspend.get());
   if (InitialSuspend.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->InitialSuspend = InitialSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeFinalSuspend() {
   // Form and check implicit 'co_await p.final_suspend();' statement.
   ExprResult FinalSuspend =
-  buildPromiseCall(*this, Fn, Loc, "final_suspend", None);
+  buildPromiseCall(S, , Loc, "final_suspend", None);
   // FIXME: Support operator co_await here.
   if (!FinalSuspend.isInvalid())
-FinalSuspend = BuildCoawaitExpr(Loc, FinalSuspend.get());
-  FinalSuspend = 

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov updated this revision to Diff 85489.
GorNishanov added a comment.

No changes. Merge with top of the tree (to simplify comparing with the updated 
version that is coming up in a second).


https://reviews.llvm.org/D28835

Files:
  include/clang/AST/StmtCXX.h
  include/clang/Sema/ScopeInfo.h
  lib/AST/StmtCXX.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include }}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -487,7 +487,7 @@
 static bool buildAllocationAndDeallocation(Sema , SourceLocation Loc,
FunctionScopeInfo *Fn,
Expr *,
-   Stmt *) {
+   Expr *) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
@@ -564,6 +564,48 @@
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema 
+  FunctionDecl 
+  FunctionScopeInfo 
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl;
+
+public:
+  SubStmtBuilder(Sema , FunctionDecl , FunctionScopeInfo , Stmt *Body)
+  : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+IsPromiseDependentType(
+!Fn.CoroutinePromise ||
+Fn.CoroutinePromise->getType()->isDependentType()) {
+this->Body = Body;
+if (!IsPromiseDependentType) {
+  PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+  assert(PromiseRecordDecl && "Type should have already been checked");
+}
+this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+makeFinalSuspend() && makeOnException() &&
+makeOnFallthrough() && makeNewAndDeleteExpr() &&
+makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }  
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
@@ -577,120 +619,159 @@
 << (isa(First) ? 0 :
 isa(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-  ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+  S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->Promise = PromiseStmt.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-  buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+  buildPromiseCall(S, , Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+  InitialSuspend = S.ActOnFinishFullExpr(InitialSuspend.get());
   if (InitialSuspend.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->InitialSuspend = InitialSuspend.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeFinalSuspend() {
   // Form and check implicit 'co_await p.final_suspend();' statement.
   ExprResult FinalSuspend =
-  buildPromiseCall(*this, Fn, Loc, "final_suspend", None);
+  buildPromiseCall(S, , Loc, "final_suspend", None);
   // FIXME: Support operator co_await here.
   if (!FinalSuspend.isInvalid())
-FinalSuspend = 

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov added a comment.

@rsmith, Looking good?


https://reviews.llvm.org/D28835



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28835: [coroutines] NFC: Refactor Sema::CoroutineBodyStmt construction.

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov created this revision.
Herald added a subscriber: mehdi_amini.

Sema::CheckCompletedCoroutineBody was growing unwieldy with building all of the 
substatements. Also, constructors for CoroutineBodyStmt had way too many 
parameters.

Instead,  CoroutineBodyStmt now defines CtorArgs structure with all of the 
required construction parameters.
CheckCompleteCoroutineBody delegates construction of individual substatements 
to short functions one per each substatement.

Also, added a drive-by fix of initializing CoroutinePromise to nullptr in 
ScopeInfo.h.
And addressed the FIXME that wanted to tail allocate extra room at the end of 
the CoroutineBodyStmt to hold parameter move expressions. (The comment was 
longer that the code that implemented tail allocation).


https://reviews.llvm.org/D28835

Files:
  include/clang/AST/StmtCXX.h
  include/clang/Sema/ScopeInfo.h
  lib/AST/StmtCXX.cpp
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp

Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s
+// RUN: %clang_cc1 -std=c++14 -fcoroutines-ts -verify %s -fcxx-exceptions
 
 void no_coroutine_traits_bad_arg_await() {
   co_await a; // expected-error {{include }}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -487,7 +487,7 @@
 static bool buildAllocationAndDeallocation(Sema , SourceLocation Loc,
FunctionScopeInfo *Fn,
Expr *,
-   Stmt *) {
+   Expr *) {
   TypeSourceInfo *TInfo = Fn->CoroutinePromise->getTypeSourceInfo();
   QualType PromiseType = TInfo->getType();
   if (PromiseType->isDependentType())
@@ -564,6 +564,48 @@
   return true;
 }
 
+namespace {
+class SubStmtBuilder : public CoroutineBodyStmt::CtorArgs {
+  Sema 
+  FunctionDecl 
+  FunctionScopeInfo 
+  bool IsValid;
+  SourceLocation Loc;
+  QualType RetType;
+  SmallVector ParamMovesVector;
+  const bool IsPromiseDependentType;
+  CXXRecordDecl *PromiseRecordDecl;
+
+public:
+  SubStmtBuilder(Sema , FunctionDecl , FunctionScopeInfo , Stmt *Body)
+  : S(S), FD(FD), Fn(Fn), Loc(FD.getLocation()),
+IsPromiseDependentType(
+!Fn.CoroutinePromise ||
+Fn.CoroutinePromise->getType()->isDependentType()) {
+this->Body = Body;
+if (!IsPromiseDependentType) {
+  PromiseRecordDecl = Fn.CoroutinePromise->getType()->getAsCXXRecordDecl();
+  assert(PromiseRecordDecl && "Type should have already been checked");
+}
+this->IsValid = makePromiseStmt() && makeInitialSuspend() &&
+makeFinalSuspend() && makeOnException() &&
+makeOnFallthrough() && makeNewAndDeleteExpr() &&
+makeReturnObject() && makeParamMoves();
+  }
+
+  bool isInvalid() const { return !this->IsValid; }  
+
+  bool makePromiseStmt();
+  bool makeInitialSuspend();
+  bool makeFinalSuspend();
+  bool makeNewAndDeleteExpr();
+  bool makeOnFallthrough();
+  bool makeOnException();
+  bool makeReturnObject();
+  bool makeParamMoves();
+};
+}
+
 void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *) {
   FunctionScopeInfo *Fn = getCurFunction();
   assert(Fn && !Fn->CoroutineStmts.empty() && "not a coroutine");
@@ -577,120 +619,159 @@
 << (isa(First) ? 0 :
 isa(First) ? 1 : 2);
   }
+  SubStmtBuilder Builder(*this, *FD, *Fn, Body);
+  if (Builder.isInvalid())
+return FD->setInvalidDecl();
 
-  SourceLocation Loc = FD->getLocation();
+  // Build body for the coroutine wrapper statement.
+  Body = CoroutineBodyStmt::Create(Context, Builder);
+}
 
+bool SubStmtBuilder::makePromiseStmt() {
   // Form a declaration statement for the promise declaration, so that AST
   // visitors can more easily find it.
   StmtResult PromiseStmt =
-  ActOnDeclStmt(ConvertDeclToDeclGroup(Fn->CoroutinePromise), Loc, Loc);
+  S.ActOnDeclStmt(S.ConvertDeclToDeclGroup(Fn.CoroutinePromise), Loc, Loc);
   if (PromiseStmt.isInvalid())
-return FD->setInvalidDecl();
+return false;
+
+  this->Promise = PromiseStmt.get();
+  return true;
+}
 
+bool SubStmtBuilder::makeInitialSuspend() {
   // Form and check implicit 'co_await p.initial_suspend();' statement.
   ExprResult InitialSuspend =
-  buildPromiseCall(*this, Fn, Loc, "initial_suspend", None);
+  buildPromiseCall(S, , Loc, "initial_suspend", None);
   // FIXME: Support operator co_await here.
   if (!InitialSuspend.isInvalid())
-InitialSuspend = BuildCoawaitExpr(Loc, InitialSuspend.get());
-  InitialSuspend = ActOnFinishFullExpr(InitialSuspend.get());
+InitialSuspend = S.BuildCoawaitExpr(Loc, InitialSuspend.get());
+