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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

This seems right to me!



================
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.
----------------
nridge wrote:
> 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?
Yes, I think this is how instantiation of non-dependent code works, e.g. in the following TransformObjCAtTryStmt:

```
  // If nothing changed, just retain this statement.
  if (!getDerived().AlwaysRebuild() &&
      TryBody.get() == S->getTryBody() &&
      !AnyCatchChanged &&
      Finally.get() == S->getFinallyStmt())
    return S;
```

I think missing this opportunity means that a non-dependent co_await will nevertheless be instantiated into a new node, which in turn means that every containing node will *also* be instantiated (because its child nodes changed during instantiation).
So this affects speed and memory usage, but I suppose not correctness.

Anyway I don't know exactly what the conditions are where it's safe to reuse the node, I suppose we leave this as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115187