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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/clang/Sema/ScopeInfo.h:138-140
+  /// \brief Whether this function has already built, or tried to build, the
+  /// the initial and final coroutine suspend points.
+  bool NeedsCoroutineSuspends : 1;
----------------
rsmith wrote:
> Is the comment here correct? It seems a slightly odd match for the member name.
The comment should probably say "True only when this function has not already built, or attempted to build, the initial and final coroutine suspend points".


================
Comment at: lib/Sema/SemaCoroutine.cpp:99
+
+  auto buildNNS = [&]() {
     auto *NNS = NestedNameSpecifier::Create(S.Context, nullptr, StdExp);
----------------
rsmith wrote:
> `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.
We can't use "%q0" in the diagnostic passed to `RequireCompleteType`, so we have to build up the elaborated `QualType` instead.


================
Comment at: lib/Sema/SemaCoroutine.cpp:112-116
+  if (S.RequireCompleteType(FuncLoc, buildNNS(),
+                            diag::err_coroutine_promise_type_incomplete))
+    return QualType();
 
   return PromiseType;
----------------
rsmith wrote:
> We used to return the `ElaboratedType` and don't do so any more.
I don't think that's true. We only build the `ElaboratedType` before issuing a diagnostic and returning `QualType()` to signal failure. 


================
Comment at: lib/Sema/SemaCoroutine.cpp:33
 /// function type.
 static QualType lookupPromiseType(Sema &S, const FunctionProtoType *FnType,
+                                  SourceLocation KwLoc,
----------------
rsmith wrote:
> rsmith wrote:
> > EricWF wrote:
> > > The changes to this function are all unrelated cleanup/improvements.
> > Just go ahead and commit these separately then.
> Please commit these changes separately if they're cleanups unrelated to the main purpose of the patch.
Ack.


================
Comment at: lib/Sema/TreeTransform.h:6802
+  return getDerived().RebuildDependentCoawaitExpr(
+      E->getKeywordLoc(), Result.get(), E->getOperatorCoawaitLookup());
+}
----------------
rsmith wrote:
> 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.)
Ack. Fixed.

Do you have an example that doesn't use a function-local definition? Since `operator co_await` cannot be defined locally.


https://reviews.llvm.org/D26057