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

Nathan Ridge via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 18 00:50:14 PDT 2022

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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list