[PATCH] D115187: [clangd] Expose CoawaitExpr's operand in the AST

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:858
   }
   ExprResult Awaitable = buildOperatorCoawaitCall(*this, Loc, E, Lookup);
   if (Awaitable.isInvalid())
----------------
sammccall wrote:
> (aside, this variable name is really unfortunate: I think `E` here is the awaitable, and `Awaitable` is the _awaiter_. If this is right, feel free to change it or not...)
I believe you're right, changed it


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:865
 
-ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *E,
-                                  bool IsImplicit) {
+ExprResult Sema::BuildResolvedCoawaitExpr(SourceLocation Loc, Expr *Operand,
+                                          Expr *E, bool IsImplicit) {
----------------
sammccall wrote:
> Not really your fault, but having two Exprs `Operand` and `E` is terribly confusing in isolation. I think `E` is `Awaiter` in standardese. Again feel free to change or not.
Agreed, changed this as well


================
Comment at: clang/lib/Sema/TreeTransform.h:7959
+
+  // FIXME: getCurScope() should not be used during template instantiation.
+  // We should pick up the set of unqualified lookup results for operator
----------------
Note, I copied this comment from [here](https://searchfox.org/llvm/rev/34a68037ddb4dff972c5d8c599cf5edf08fadf6b/clang/lib/Sema/SemaStmt.cpp#2580).


================
Comment at: clang/lib/Sema/TreeTransform.h:7934
 TreeTransform<Derived>::TransformCoawaitExpr(CoawaitExpr *E) {
-  ExprResult Result = getDerived().TransformInitializer(E->getOperand(),
-                                                        /*NotCopyInit*/false);
-  if (Result.isInvalid())
+  // XXX is transforming the operand and the common-expr separately the
+  // right thing to do?
----------------
sammccall wrote:
> Ah, this doesn't *sound* right - it's going to create duplicate subexprs I think, and we should really try to have the operand still be a subexpr of the commonexpr like in the non-instantiated case.
> 
> TransformInitListExpr() is a fairly similar case, and it just discards the semantic form and rebuilds from the syntactic one. TransformPseudoObjectExpr seems to be similar.
> 
> I suppose the analogous thing to do here would be to have RebuildCoawaitExpr call Build**Un**resolvedCoawaitExpr with the operand only, but this is a large change! (And suggests maybe we should stop building the semantic forms of dependent coawaits entirely, though that probably would regress diagnostics).
> Maybe worth giving this a spin anyway?
> 
> @rsmith, care to weight in here?
> I suppose the analogous thing to do here would be to have RebuildCoawaitExpr call Build**Un**resolvedCoawaitExpr with the operand only,

I gave this a try.


================
Comment at: clang/lib/Sema/TreeTransform.h:7947
 
   // Always rebuild; we don't know if this needs to be injected into a new
   // context or if the promise type has changed.
----------------
sammccall wrote:
> (FWIW I don't know what "injected into a new context" means, but I don't think the promise type can change since DependentCoawaitExpr was added in 20f25cb6dfb3364847f4c570b1914fe51e585def.)
> 
Is the implication of this that there are some conditions under which we can reuse the original CoawaitExpr rather than rebuilding it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115187/new/

https://reviews.llvm.org/D115187