[PATCH] D26057: [coroutines] Add CoawaitDependentExpr AST node and use it to properly build await_transform.

[Alexey Bataev via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/Sema/SemaCoroutine.cpp:237-244
+static UnresolvedSet<16> lookupOperatorCoawaitCall(Sema &SemaRef, Scope *S,
+                                                   SourceLocation Loc,
+                                                   Expr *E) {
   UnresolvedSet<16> Functions;
   SemaRef.LookupOverloadedOperatorName(OO_Coawait, S, E->getType(), QualType(),
                                        Functions);
+  return Functions;
----------------
EricWF wrote:
> ABataev wrote:
> > Maybe it is better to add an argument `UnresolvedSetImpl &OpCandidates`?
> This seems like the best place to specify the concrete type now that everything else uses `UnresolvedSetImpl`.
Does really matter that it is `UnresolvedSet<16>`, but not `UnresolvedSet<8>` or `UnresolvedSet<4>`? If not, you should not use `UnresolvedSet<16>` as the param type or return type


================
Comment at: lib/Sema/SemaCoroutine.cpp:301
+                                   SourceLocation Loc, StringRef Name,
+                                   MutableArrayRef<Expr *> Args) {
+  assert(Coroutine->CoroutinePromise && "no promise for coroutine");
----------------
EricWF wrote:
> ABataev wrote:
> > Why do you need `MutableArrayRef<Epr *>`, when `ArrayRef<Expr *>` is enough? Also `buildMemberCall` must be changed
> `Sema::ActOnCallExpr` requires the `MutableArrayRef`, so I don't see how this can be changed.
Ok, then maybe it is better to use `MultiExprArg` rather than `MutableArrayRef<Expr *>` which is looking a bit confusing


https://reviews.llvm.org/D26057