[PATCH] D25879: [coroutines] Add allocation and deallocation substatements.

[Alexey Bataev via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Sema/SemaCoroutine.cpp:165
 
+static Expr *buildBuiltinCall(Sema &S, SourceLocation Loc, Builtin::ID id,
+                              MutableArrayRef<Expr *> CallArgs) {
----------------
`id` does not fllow LLVM coding rules. Must be `Id`


================
Comment at: lib/Sema/SemaCoroutine.cpp:169
+  LookupResult R(S, &S.Context.Idents.get(Name), Loc, Sema::LookupOrdinaryName);
+  S.LookupName(R, S.TUScope, true);
+
----------------
`true` argument must have adjacent comment with the name of the parameter, like `/*Param=*/true`


================
Comment at: lib/Sema/SemaCoroutine.cpp:171
+
+  FunctionDecl *BuiltInDecl = R.getAsSingle<FunctionDecl>();
+  assert(BuiltInDecl && "failed to find builtin declaration");
----------------
`auto *BuiltInDecl`


================
Comment at: lib/Sema/SemaCoroutine.cpp:174-175
+
+  ExprResult DeclRef = S.BuildDeclRefExpr(BuiltInDecl, BuiltInDecl->getType(),
+                                          VK_RValue, Loc, nullptr);
+  assert(DeclRef.isUsable() && "Builtin reference cannot fail");
----------------
I believe you can drop the last `nullptr` arg, it is defaulted. And I think it's better to make it `VK_LValue`


================
Comment at: lib/Sema/SemaCoroutine.cpp:409
+
+  CXXRecordDecl *PointeeRD = PromiseType->getAsCXXRecordDecl();
+  assert(PointeeRD && "PromiseType must be a CxxRecordDecl type");
----------------
`auto *PointeeRD`


================
Comment at: lib/Sema/SemaCoroutine.cpp:472-473
+
+  ExprResult NewExpr = S.ActOnCallExpr(S.getCurScope(), NewRef.get(), Loc,
+                                       FrameSize, Loc, nullptr);
+  if (NewExpr.isInvalid())
----------------
The last `nullptr` can be dropped


================
Comment at: lib/Sema/SemaCoroutine.cpp:481
+
+  QualType opDeleteQualType = OperatorDelete->getType();
+
----------------
`OpDeleteQualType`


================
Comment at: lib/Sema/SemaCoroutine.cpp:494
+  // Check if we need to pass the size.
+  const FunctionProtoType *opDeleteType =
+      opDeleteQualType.getTypePtr()->getAs<FunctionProtoType>();
----------------
`const auto *OpDeleteType`


================
Comment at: lib/Sema/SemaCoroutine.cpp:496-498
+  if (opDeleteType->getNumParams() > 1) {
+    DeleteArgs.push_back(FrameSize);
+  }
----------------
No need for braces in one-line substatement


================
Comment at: lib/Sema/SemaCoroutine.cpp:501
+  ExprResult DeleteExpr = S.ActOnCallExpr(S.getCurScope(), DeleteRef.get(), Loc,
+                                          DeleteArgs, Loc, nullptr);
+  if (DeleteExpr.isInvalid())
----------------
Drop last `nullptr`


https://reviews.llvm.org/D25879