[PATCH] D26057: [coroutines] Add DependentCoawaitExpr and fix re-building CoroutineBodyStmt.

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: include/clang/AST/ExprCXX.h:4237
+  /// compiler.
+  bool IsImplicitlyCreated : 1;
+
----------------
I would go with just `isImplicit`, to match other similar uses throughout the AST. Also maybe sink this into the `Stmt` bitfields to make this class 8 bytes smaller


================
Comment at: lib/AST/ItaniumMangle.cpp:3302-3303
   case Expr::AddrLabelExprClass:
+  // This should no longer exist in the AST by now
+  case Expr::DependentCoawaitExprClass:
   case Expr::DesignatedInitUpdateExprClass:
----------------
I don't think "should no longer exist" is true. If `co_await` can appear in a function signature at all, it can appear with a dependent operand. This should be mangled the same as a non-dependent `co_await` expression.


================
Comment at: lib/Sema/SemaCoroutine.cpp:33
 /// function type.
 static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
+                                  SourceLocation KwLoc,
----------------
EricWF wrote:
> The changes to this function are all unrelated cleanup/improvements.
Just go ahead and commit these separately then.


================
Comment at: lib/Sema/SemaCoroutine.cpp:99
+
+  auto buildNNS = [&]() {
     auto *NNS = NestedNameSpecifier::Create(S.Context, nullptr, StdExp);
----------------
`buildElaboratedType` would be a better name for what this does. I also wonder if this is really necessary, or whether we can just use %q0 in the diagnostic format to request a fully-qualified type name.


================
Comment at: lib/Sema/SemaCoroutine.cpp:112-116
+  if (S.RequireCompleteType(FuncLoc, buildNNS(),
+                            diag::err_coroutine_promise_type_incomplete))
+    return QualType();
 
   return PromiseType;
----------------
We used to return the `ElaboratedType` and don't do so any more.


================
Comment at: lib/Sema/SemaCoroutine.cpp:409-414
+  return BuildDependentCoawaitExpr(Loc, E,
+                                   cast<UnresolvedLookupExpr>(Lookup.get()));
+}
+
+ExprResult Sema::BuildDependentCoawaitExpr(SourceLocation Loc, Expr *E,
+                                           UnresolvedLookupExpr *Lookup) {
----------------
This seems like an odd naming choice. I'd expect `BuildDependentCoawaitExpr` to only deal with the case where the expression is dependent (and to never be called otherwise), and `BuildCoawaitExpr` to handle both the case where the expression is dependent and the case where it is non-dependent. Maybe the other function should be called `BuildResolvedCoawaitExpr` or similar.


================
Comment at: lib/Sema/SemaDecl.cpp:11386
 
-  // If we don't have a visible definition of the function, and it's inline or
+  // If we don't have a viNsible definition of the function, and it's inline or
   // a template, skip the new definition.
----------------
Stray change.


================
Comment at: lib/Sema/TreeTransform.h:6687-6689
+  // Set that we have (possibly-invalid) suspend points before we do anything
+  // that may fail.
+  ScopeInfo->setCoroutineSuspendsInvalid();
----------------
Please use a term other than "invalid" here. We generally use "invalid" to mean "an error has been diagnosed, use best-effort recovery".


================
Comment at: lib/Sema/TreeTransform.h:6802
+  return getDerived().RebuildDependentCoawaitExpr(
+      E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}
----------------
You need to transform the UnresolvedLookupExpr here too. (Example: we might have a function-local declaration of `operator co_await` that needs to be transformed into the version in the instantiated function.)


https://reviews.llvm.org/D26057