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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a subscriber: rsmith.
sammccall added a comment.

This generally looks really good, I'm worried about the separate transformation in the template case though.



================
Comment at: clang/include/clang/AST/ExprCXX.h:4731
 
+  Expr *getOperand() const {
+    return static_cast<Expr *>(SubExprs[SubExpr::Operand]);
----------------
I think a comment here like `// The syntactic operand written in the code` or so would help clarify the distinction between this and the common/ready/suspend/resume family.

I'm just thinking of all the times working on tooling when you're trying to understand how to handle every node type without deep-diving into the semantics of each one :-)

Also maybe group getOperand(), getKeywordLoc(), getBeginLoc(), getEndLoc() together?


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:737
       return StmtError();
-    Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(),
+    Suspend = BuildResolvedCoawaitExpr(Loc, Suspend.get(), Suspend.get(),
                                        /*IsImplicit*/ true);
----------------
I'm not sure Suspend.get() here is the most appropriate value for Operand.

It's a little abstract, since we're talking about the syntactic operand of an implicit call :-)
But for consistency with the explicit case, I think the operand should be the result of `initial_suspend()` etc, *before* we call `operator co_await`.

i.e. the value of Suspend.get() a couple of lines above.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:838
 
+  Expr *Operand = E;
+
----------------
This has become confusing I think. The old code was was changing the meaning of the `E`, because we didn't need the original anymore. Rather than introduce a new variable for the original meaning, I'd prefer to introduce one for the changed meaning:

```
// line 848
auto *Transformed = E;
if (lookupMember(...)) {
  ...
  Transformed = R.get();
}
buildOperatorCoAwaitCall(..., Transformed);
```

(if you also want to rename E -> Operand, that makes sense to me too!)


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:858
   }
   ExprResult Awaitable = buildOperatorCoawaitCall(*this, Loc, E, Lookup);
   if (Awaitable.isInvalid())
----------------
(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...)


================
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) {
----------------
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.


================
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?
----------------
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?


================
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.
----------------
(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.)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115187